Update and expand events Cypress tests. DDFHER-30 #1649
Conversation
By using `->label()`, the title will never be overwritten when updating the instance level - this fixes that issue.
This includes creating events, making sure the admin flow works. It also includes checking that data shows up in the API, that it is able to be PATCHed and that changes are reflected in the API.
There was a problem hiding this comment.
I appreciate the effort put into this but I think there are some significant issues that needs to be addressed before this is ready.
They primarily revolve around:
- Code structure
- Cypress practices
- Usage of existing structures
We previously talked about how the test of the Event API was related to the test of the Event UI and how code could be structured to be reused across both.
As I see it this drops the existing test of the Event UI entirely to replace it with a test of the UI. I think that is a shame. I wonder how it turned out this way.
I have provided some notes. Let's talk it through in person.
| cy.get(`[name="${fieldKey}[0][value][date]"]`) | ||
| .type(event.start.format("YYYY-MM-DD")).focus(); | ||
|
|
||
| cy.wait(1000); |
There was a problem hiding this comment.
Please reconsider whether it is necessary to use cy.wait().
It is considered an antipattern. Even if it takes a bit before the application code sets the end date Cypress should wait for that to occur by default.
| cy.get(`[name="${fieldKey}[0][end_value][date]"]`) | ||
| // Checking that the end date is pre-filled, based on start date. | ||
| .should("have.value", event.start.format("YYYY-MM-DD")) | ||
| .type(event.end.format("YYYY-MM-DD")); |
There was a problem hiding this comment.
Please reconsider the structure here.
You have a function setEventSeriesDate() where the name signals setting a value. With this addition it also does something else.
I suggest splitting this into smaller functions. I also suggest putting the check for whether the event end date and time are prefilled into a separate test as it was the case before.
| // Recurring_event throws weird exceptions here, that we want to avoid failing | ||
| // the whole cypress test. | ||
| Cypress.on('uncaught:exception', () => { | ||
| // returning false here prevents Cypress from | ||
| // failing the test | ||
| return false | ||
| }) |
There was a problem hiding this comment.
I appreciate the comment but this does not seem like a good thing.
I am going to check this in action before I decide on whether it is necessary.
| if (!event.ticketManagerRelevance) { | ||
| cy.contains('Show sidebar panel').click(); | ||
| cy.findByLabelText("Relevant for ticket manager").click(); | ||
| cy.contains('Close sidebar panel').click(); | ||
| } | ||
|
|
||
| if (!event.status) { | ||
| cy.get('[data-drupal-selector="edit-status-value"]').click(); | ||
| } | ||
|
|
||
| cy.findByLabelText("Recur Type").select(event.recurType, { | ||
| // We have to use force when using Select2. | ||
| force: true, | ||
| }); |
There was a problem hiding this comment.
Is there a reason why you went for data attributes instead of labels here?
To me the relevance for ticket manager and recur type setting are easier to read as they rely on UI texts using findByLabelText() and the like.
The test works locally (when setting the test users to prefered language english..) but fails in Github. Looks like a timezone issue, but I dont have time to fix it.