Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Update and expand events Cypress tests. DDFHER-30 #1649
base: develop
Are you sure you want to change the base?
Update and expand events Cypress tests. DDFHER-30 #1649
Changes from all commits
ca86bcf
bdbd174
1cca3b7
fbaa5c0
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Check failure on line 2 in cypress/e2e/events.cy.ts
GitHub Actions / Lint Cypress tests
Check failure on line 5 in cypress/e2e/events.cy.ts
GitHub Actions / Lint Cypress tests
Check failure on line 9 in cypress/e2e/events.cy.ts
GitHub Actions / Lint Cypress tests
Check failure on line 10 in cypress/e2e/events.cy.ts
GitHub Actions / Lint Cypress tests
Check failure on line 11 in cypress/e2e/events.cy.ts
GitHub Actions / Lint Cypress tests
Check failure on line 12 in cypress/e2e/events.cy.ts
GitHub Actions / Lint Cypress tests
Check failure on line 13 in cypress/e2e/events.cy.ts
GitHub Actions / Lint Cypress tests
Check failure on line 14 in cypress/e2e/events.cy.ts
GitHub Actions / Lint Cypress tests
Check failure on line 15 in cypress/e2e/events.cy.ts
GitHub Actions / Lint Cypress tests
Check failure on line 16 in cypress/e2e/events.cy.ts
GitHub Actions / Lint Cypress 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.
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.
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.
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.
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 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.
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.
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.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.
Consider if setting the recur type should be a part of the date setting.
As far as I can tell
setEventSeriesDate()
depends on the recur type being set.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.
Please use
cy.api()
for such requests.It a plugin that we use elsewhere for these things. It makes managing API requests in test cases easier.
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.
Please reconsider your code structure.
This is a find method. I suggest putting your assertions in a separate function.
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.
Please reconsider this approach.
I would suggest a function which, based on an
EventType
found the event in the event list based on the title and went to the edit page from there.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 seems like a reocurring approach to generating a random string.
Please put it in a separate function or create a factory method which provides an event ready for testing.
In general I wonder why you went with this approach when other tests use explicit values for individual cases.
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 cleans up after each test. Cypress recommends cleaning up before.
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 suggest putting this in a separate test.
It seems to deal with a separate issue.