Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

SFINT-2878 Added Cypress functional tests #60

Merged
merged 3 commits into from
Feb 25, 2020

Conversation

jeremierobert-coveo
Copy link
Collaborator

I added Cypress to run functional test in the repo.

Here some screenshot.

image

And here too

image

@louis-bompart
Copy link
Collaborator

Can you split the PR in two?

  • The setup
  • The tests

So we can distinguish the setup and the tests in our review (and upcoming /blame :P )

Copy link
Collaborator

@louis-bompart louis-bompart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed setup, LGTM!
Would split the tests in another PR to have small scoped PR and easily reversible PRs 🎉

@jeremierobert-coveo
Copy link
Collaborator Author

imo, the setup is pointless with at least a single test to test the test setup.
And those User actions tests where quick and simple to write.

@louis-bompart
Copy link
Collaborator

imo, the setup is pointless with at least a single test to test the test setup.
And those User actions tests where quick and simple to write.

The thing is if your tests become an issue, we will have a single commit to revert without having to untangle what to keep, what to ditch.

It keeps things contained. Like, your setup should not be hard-linked with a feature

@jeremierobert-coveo jeremierobert-coveo force-pushed the test/SFINT-2878 branch 2 times, most recently from 19a5585 to 663b541 Compare February 24, 2020 22:30
@jeremierobert-coveo
Copy link
Collaborator Author

Well I'll need to add a dummy then
image

@louis-bompart
Copy link
Collaborator

Well I'll need to add a dummy then
image

Yep, do a dummy, remove it in the PR that include the first FT

@jeremierobert-coveo
Copy link
Collaborator Author

Done

@louis-bompart
Copy link
Collaborator

Why not using TypeScript tests?
https://docs.cypress.io/guides/tooling/typescript-support.html#Configure-tsconfig-json
https://github.com/bahmutov/add-typescript-to-cypress

It could allow us to refer to some code from our src easily (Like the classnames)

@jeremierobert-coveo
Copy link
Collaborator Author

Here some typescript sprinkle

Copy link
Contributor

@nathanlb nathanlb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice! (Borat voice)

Copy link
Collaborator

@louis-bompart louis-bompart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apart from the modification pages/index.html → pages/attached_result.html, LGTM.

Copy link
Collaborator

@mikegron mikegron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@jeremierobert-coveo jeremierobert-coveo merged commit 1b49530 into master Feb 25, 2020
@jeremierobert-coveo jeremierobert-coveo deleted the test/SFINT-2878 branch February 25, 2020 16:09
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants