-
Notifications
You must be signed in to change notification settings - Fork 16
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
feat: add cypress end to end tests #725
Conversation
30d651d
to
d0dec6b
Compare
should call e2e tests something else, functional? smoke? browser? integration? add initial smoke tests
cedfe45
to
273ee20
Compare
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.
Comments attached, what do you think @sgfost ?
it("should browse the model library and be able to enter a search query", () => { | ||
cy.visit("/"); // visit homepage | ||
cy.visit("/codebases"); | ||
assert(cy.get("h1").contains("Computational Model Library")); |
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.
should we also make assertions on an expected model to find in the model library?
// Navigate to the first 'search-result', find the 'a' tag within the 'title' div, and click it | ||
cy.get(".search-result", { timeout: 10000 }) | ||
.first() | ||
.find(".title a") |
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.
this filtering criteria seems poorly vague for identifying a codebase download. Setup a more precise criteria even if you need to include additional metadata on the model library page.
e2e/cypress/tests/codebase.spec.ts
Outdated
|
||
it("should be able to download a codebase", () => { | ||
cy.visit("/codebases"); | ||
cy.get(".search-result").first().find("a").first().click(); |
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.
How brittle is this criteria? Is the first a href after a .search-result
CSS class targeted enough to always get the first codebase? Or should we make all codebase results more explicit with a CSS class or identifier and target that explicitly?
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 think this and the above comment are the same issue, should probably just use data attributes to target this
e2e/cypress/tests/codebase.spec.ts
Outdated
it("should verify that the codebase was uploaded correctly", () => { | ||
cy.visit("/codebases"); | ||
cy.get(".search-result").first().find("a").first().click(); | ||
assert(cy.get("h1").contains("Codebase Title")); |
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 kinds of assertions are highly dependent on whatever stylistic or textual changes we want to make in the frontend. It might be better to have a list or other data structure of expected text and programmatically iterate through it with assertions and comment the data structure as a place of possible future 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.
Good idea, perhaps a global object that describes the initial database state which can be modified at runtime? for example:
{
...
codebases: [
{ title: "Codebase Title", description: "something" },
],
users: [
{ username: "test_user", password: "123456", ... }
],
...
}
and then, as Allen said, use this to assert that certain things are found on a page, rather than at some specific location in the DOM tree
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 @CharlesSheelam. I think the main thing remaining is fragility in selectors.
For cases where we need to select an element to perform some action (like grabbing a codebase search result and the link to it's detail page) it should look for a data attribute rather than a css class/first element/etc.
And for cases where we're asserting that something exists, I would say just expand the search to be much more broad (whole page or perhaps an existing container with a data attribute) and use a global data structure that describes the content of the database to define what we want to check for
runs in a test environment with built frontend assets and a completely separate db container (is there a more elegant way to achieve isolation here? more similar to django's test db setup?) always restores from minimal borg repo actual cypress tests are run with cypress github action and on failure, screen recordings are saved as artifacts
tests are failing when running on the GH runner, partially because I disabled the debug toolbar but there are a few other things it seems to be getting tripped up on. They mostly seem to coincide with selectors mentioned above that should get changed anyways |
* setup shouldn't be run as a test * fixed a couple fragile selectors by removing dependencies on classes/ids/element trees subject to change
@CharlesSheelam I did a little bit of cleanup/refactoring. Just a couple things before I think this is good to go:
|
0334f94
to
2fe98f6
Compare
- apply prettier - fix industry options selector - apply style to frontend
589389b
to
11bcf8e
Compare
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.
LGTM, only issue is the data-cy attribute values are sometimes space cased or kebab-cased, should stick to one convention. Can fix in a future refactor
* Get an element by its data-cy attribute | ||
* @param {string} value - value of the data-cy attribute | ||
*/ | ||
export function getDataCy(value: string): Cypress.Chainable { |
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 get why this is named because it retrieves the "data-cy" attribute but getCypressData(attributeValue: string)
might be a better name overall. To be considered by future devs, not a blocker on this PR
e2e/cypress/tests/codebase.spec.ts
Outdated
getDataCy("codebase title").type(codebase.title); | ||
getDataCy("codebase description").type(codebase.description); | ||
getDataCy("codebase replication-text").type(codebase["replication-text"]); | ||
getDataCy("codebase associated publications").type(codebase["associated-publications"]); |
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.
inconsistency here, why is replication-text hyphenated if "associated publications" is not? based on the rest of the attribute names I think we should either use spaces or kebab-case for all of em
failing e2e tests is due to programming error on my part in #757. I suppose it illustrates the usefulness of these! |
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 @sgfost and @CharlesSheelam !
* removed unused class attr
Working on smoke testing to test functionality of the comses website.