-
Notifications
You must be signed in to change notification settings - Fork 7
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
First Cypress End to End Testing Implementation #165
Conversation
@@ -22,6 +22,7 @@ | |||
<b-nav-item | |||
v-for="(navItem, _key) in navItems" | |||
:active="pageName === navItem.fullName" | |||
:data-cy="'menu-item-' + navItem.pageName" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As per recommended best practice, any attribute accessed as an identifier by cypress should be separate from identifiers used by the app itself in order to avoid brittleness.
Our current standard is data-cy
for the attribute name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense to me. Only question here is: would you be OK with changing navItem.fullName
independently of navItem.pageName
? I.e. would we ever allow the $store.pageData.categorization.pageName = "categorization"
page to have a UI name of "Category Assignment" without also changing the pageName
? Because if not, this might be a case where selecting by UI text might be more intuitive
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I considered whether to select by ID value or UI label too – if I'm understanding your comment correctly.
Whatever we do should be standard in our testing implementation, but my intuition is not to rely on the label text but rather than the html attribute value. Arguably, it is less likely for the attribute value to change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, then let's keep it as is with the data-cy
attribute selectors.
cypress/support/commands.js
Outdated
@@ -93,7 +93,7 @@ Cypress.Commands.add("nextPageByNav", (p_navItemName) => { | |||
}); | |||
|
|||
// Go through index page and select participants tsv and json dictionary files, advance to categorization page | |||
Cypress.Commands.add("passIndex", () => { | |||
Cypress.Commands.add("passIndex", (p_dataTableFilepath, p_dataDictionaryFilepath) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These pass
commands now take variable input paths from the current testing context.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand this correctly, the pass
functions are only used by the ui
class of tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's correct. The other reason I'm mentioning these functions - and I'll continue this thought in your other general comment on UI-based testing - is that these pass functions may be rendered moot depending on how we decide to configure the UI e2e test(s).
More on that in your general comment on the PR.
Page/UI tests for annotation and download pages will be added in an upcoming commit for this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @jarmoza for setting up all of these tests. Very excited that we have a test suite now and can keep expanding it! This is a super important step for us to add new features and have confidence in the annotator. Very cool!
I have made some comments in the code, please take a look or let me know what you think.
Two more general comments:
1 - I don't understand the distinction between page and ui
(I saw your comment just commenting here to keep things together). Both are e2e tests. We always want to test the UI, and we never want to test the programmatic state setting. The only difference to me is that we want to test a route through the UI only once. And once we know it works, we can take the programmatic shortcut for any subsequent tests. So for example: we only test the select-and-upload functionality on the index.vue
page once, and for subsequent tests we "upload" the file not via the UI but programmatically.
All that is to say: let's merge page
and ui
and make sure that each UI route is tested at least once, and then used programmatically afterwards.
2 - Let's keep it simple as long as we can
We're not using many different example data files yet, so I don't think we need the added complexity of fixtures and aliases in the beforeEach
sections of the tests (I made separate inline comments about these). We're also not using component testing yet, so let's remove the config files for that to keep things clean. We can always re-generate them once we need them. And lastly, this is a big PR - I understand this is a big piece of work. But let's try to aim for smaller, more incremental changes in the future. They are easier to review and and we can get faster feedback and discuss things. It also makes for more logical PRs, because if it's open for a long time, you can get old solutions from earlier in the PR that live on in some commented out piece of code and so on. We can always add more stuff later if we need it!
@@ -22,6 +22,7 @@ | |||
<b-nav-item | |||
v-for="(navItem, _key) in navItems" | |||
:active="pageName === navItem.fullName" | |||
:data-cy="'menu-item-' + navItem.pageName" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense to me. Only question here is: would you be OK with changing navItem.fullName
independently of navItem.pageName
? I.e. would we ever allow the $store.pageData.categorization.pageName = "categorization"
page to have a UI name of "Category Assignment" without also changing the pageName
? Because if not, this might be a case where selecting by UI text might be more intuitive
cypress.config.js
Outdated
|
||
baseUrl: "http://localhost:3000", | ||
|
||
component: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd check this again. To me this looks as if the "component" test config is nested inside the e2e config: https://docs.cypress.io/guides/references/configuration#Testing-Type-Specific-Options
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup. Good catch. And I'm guessing it didn't error because Cypress would not be looking for a component
property in the e2e
definition.
|
||
// Link the second column in the column select table with the 'Assessment Tool' category | ||
cy.get("[data-cy='column-linking-table'] tbody tr td") | ||
.contains("iq") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just use row index selector / random row
|
||
// Link the second column in the column select table with the 'Assessment Tool' category | ||
cy.get("[data-cy='column-linking-table'] tbody tr td") | ||
.contains("session") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- use any row here or based on index to not tie to specific example data file
- comment doesn't match the test (i.e. we're categorizing as "subject id" here, not as "assessment tool")
@@ -0,0 +1,167 @@ | |||
describe("tests on the index page via store interaction", () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I disagree here. We do want to have the ability to programmatically set the store state without using the UI for other tests! But we have to test the UI route at least once to be certain that it actually works. And this suite of tests would be the place for that! I.e. here we have to use the file selector and the upload button and all of that jazz, not the programmatic access.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@surchs Can you clarify? I've currently separated out programmatic state tests by page. In this case, we have the rather simple index page where state is loaded and we test to see if that state load results in the desired access to the categorization page.
I could see removing this file though and just relying on the UI e2e test entirely for this page. Is that what you mean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, exactly. We want to make sure that the path a user can take here works as expected. So we need to test that "click button, select file, upload file" has the expected consequence of "now a button is no longer disabled". A user wouldn't directly interact programmatically with the store, so we don't need to test whether a programmatic store manipulation would have the expected effect on the UI.
The tests we want for the index page are already implemented in index-uitests.cy.js
, so I agree with removing this file.
@@ -0,0 +1,5 @@ | |||
{ | |||
"source_folder": "examples/good/", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really like the idea of parametrizing these tests. I am just not sure how we would do that with these config files and the test spec. I would say that for now we stick to a simple case where we only have one kind of example file. And then we can find a way to use different example files in a separate PR.
cypress/support/commands.js
Outdated
@@ -93,7 +93,7 @@ Cypress.Commands.add("nextPageByNav", (p_navItemName) => { | |||
}); | |||
|
|||
// Go through index page and select participants tsv and json dictionary files, advance to categorization page | |||
Cypress.Commands.add("passIndex", () => { | |||
Cypress.Commands.add("passIndex", (p_dataTableFilepath, p_dataDictionaryFilepath) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand this correctly, the pass
functions are only used by the ui
class of tests?
@surchs I think this PR is almost fully baked. I will follow up with commit(s) based on your comments and request review. (As you've mentioned, the PR is already large so any discussed future changes should be for the next PRs.) Just wanted to make some general comments re: our PR process and the UI-based testing for upcoming PR(s)
|
For some reason I can't find the comment on moving setup code to the support file |
@surchs Okay. Good to go now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the updates @jarmoza and the changes!
I'll reply to your comment in here quickly:
- Regarding the longer PR, I think this is a good chance to discuss how we would like initial (large) features to be added to our repos for ourselves and future collaborators
Agreed, let's discuss this today in person. That's a better format for it.
- Regarding the UI based testing, there are several ways we can go with this and my thought on the implementation has changed the more I've worked with Cypress.
Totally understandable. As they are right now, I think the distinction of page
and ui
tests is not needed and should be combined. The goal is to test paths through the app from a users perspective, so we should test the UI. And because the later pages depend on state set by previous ones, it also makes sense to not go through the UI route to set this stage every time. But we also don't need to test the programmatic state setting itself.
I think the strategy here is to test every route through the app via the UI at least once. And then use programmatic state setting for subsequent tests that build on it. So for example: test that we can click the upload button on the index page, select a file, have it upload and validate ... all via the UI once. And then when we test the categorization page, set the store state for this test (i.e. the dataTable and dataDictionary) programmatically, but test the categorization page again via the UI (i.e. clicking on categories and column names ...).
So I'd say:
- let's keep
index-uitests.cy.js
because it is a UI based test of the index page - remove
index-pagetest.cy.js
because it is a direct copy of 1. that test programmatic store changes. - let's keep
categorization-pagetests.cy.js
because it is a UI test of the categorization page that handles the index-page based state setting programmatically and not via the UI - remove
categorization-uitests.cy.js
because it is a direct copy of 2 that sets initial state by re-testing the UI route through the index page (although that's abstracted away into a separate function).
With that, let's remove all of the pass_...
functions from cypress/support/commands.js
and refactor the tests themselves to live under cypress/e2e
and drop the -pagetests
or -uitests
suffix.
Take a look at any of the comments that are still open and then with these changes, I think this is good to go! 🎉 we'll have our test suite to build out!
@@ -22,6 +22,7 @@ | |||
<b-nav-item | |||
v-for="(navItem, _key) in navItems" | |||
:active="pageName === navItem.fullName" | |||
:data-cy="'menu-item-' + navItem.pageName" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, then let's keep it as is with the data-cy
attribute selectors.
@@ -0,0 +1,5 @@ | |||
{ | |||
"source_folder": "examples/good/", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK great. In that case we can remove this file here for the current PR.
@@ -0,0 +1,167 @@ | |||
describe("tests on the index page via store interaction", () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, exactly. We want to make sure that the path a user can take here works as expected. So we need to test that "click button, select file, upload file" has the expected consequence of "now a button is no longer disabled". A user wouldn't directly interact programmatically with the store, so we don't need to test whether a programmatic store manipulation would have the expected effect on the UI.
The tests we want for the index page are already implemented in index-uitests.cy.js
, so I agree with removing this file.
.click(); | ||
|
||
// Assert 2 - Nav and next button should be enabled | ||
// Assert 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. But here you have both Assert 2
and Assert 1
, and I am not sure what that refers to. I like the Assert
comments where the reason behind the expectation isn't obvious from the code. E.g. Nav and next button should not yet be enabled
is helpful, because I don't know what the app should do after I click on "Subject ID". I would even consider adding: " because no column has been categorized yet"
or something like that.
On the other hand, I don't find the comments about actions helpful, because they restate what follows in the code (e.g. "Select subject ID category"
). So I would consider getting rid of them
|
||
// Link the first column in the column select table with the 'Subject ID' category | ||
cy.get("[data-cy='column-linking-table'] tbody tr td") | ||
.contains("participant_id") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the issue. Just to be clear: my comments refer to the [data-cy='column-linking-table']
selector, aka the thing on the right. There I suggest not selecting based on text because that's brittle. We expect there to be a column called "participant_id"
, but what if we end up using an example that calls this "subject_number"
. We wouldn't want the test to fail because of that.
So my point is: let's just pick any column on the right (e.g. the first one) because at this point our app doesn't care if we picked the right one as long as one is picked.
|
||
// Link the second column in the column select table with the 'Diagnosis' category | ||
cy.get("[data-cy='column-linking-table'] tbody tr td") | ||
.contains("group") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above: I'm not referring to [data-cy='categorization-table']
here where I think text-based selection makes sense (after all, we control the category names). I'm talking about the [data-cy='column-linking-table']
selector (for table on the right)
} | ||
}, | ||
|
||
component: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if I missed this section the first time. It's in the right place. But since we aren't using the component framework, I think we should add that when we start using it and remove it for now.
Thanks for the clarification. Makes sense to me and agreed! |
Last commits are up for this PR. I've done a rework of the files including the requested modifications from @surchs with some additions. The new organization in the The second addition is a place for future dynamic testing using different datasets. I've retained the ability to load different data sets through a json config file, but am now using the recommend way of doing such tests as seen here (https://github.com/cypress-io/cypress-example-recipes/tree/master/examples/fundamentals__dynamic-tests), and more specifically this example (https://github.com/cypress-io/cypress-example-recipes/blob/master/examples/fundamentals__dynamic-tests/cypress/e2e/fixture-spec.cy.js). A list of dataset config json are added in an array via Included in the cleanup aside from standardization of testing and commenting are some helper functions in Old page-specific "UI" test files are now removed in a141281. |
One additional note: There is currently a bug with the |
Hey @jarmoza. Looks good to me! Congrats on this implementation, that's a big step 🎉! |
This PR is being set up for discussion of our first implementation of end to end testing using Cypress.
Comments on commits thus are added below.