Skip to content
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

[Testing:Developer] General Cypress test improvements #7695

Merged
merged 27 commits into from
Apr 29, 2022

Conversation

DefinitiveAbove
Copy link
Member

@DefinitiveAbove DefinitiveAbove commented Mar 2, 2022

What is the current behavior?

Some of the course material tests in Cypress were very flaky, and the visit command is missing a return leading to undefined behavior.

What is the new behavior?

Added return for visit command overwrite (https://docs.cypress.io/api/cypress-api/custom-commands#Overwrite-visit-command) - breaks some of the tests requiring some of the fixes listed below.

Added waitPageChange helper command - waits for the page to change once to a new page (navigation/refresh) before continuing code execution - used in login/logout and multiple other locations where appropriate to avoid race conditions.

Added force onto multiple timestamp-related field entries (tests would fail depending on screen size because the date picker would be covering the typing box).

Added logout to afterEach for test cases in general (visit fix led to Cypress resetting cookies - signing out most of the time, but not always (some sort of bug having to do when the test before the current test fails, leading to Cypress not signing out)).

Fixes focused around the course materials tests to decrease inconsistencies/race conditions, removed waits/reloads where unneeded after fixes.

Other information?

Test that doesn't run in CI is failing - Pre-autograding test - Should show newly added autograding jobs (autograding_status_1.spec.js); not sure what the correct numbers are supposed to be since the numbers keep changing on my local instance.

@codecov
Copy link

codecov bot commented Mar 2, 2022

Codecov Report

Merging #7695 (2d4472b) into master (7ae654a) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #7695   +/-   ##
=========================================
  Coverage     21.82%   21.82%           
  Complexity     7490     7490           
=========================================
  Files           198      198           
  Lines         24423    24423           
  Branches         54       54           
=========================================
  Hits           5331     5331           
  Misses        19038    19038           
  Partials         54       54           
Flag Coverage Δ
autograder 16.50% <ø> (ø)
js 32.88% <ø> (ø)
migrator 99.20% <ø> (ø)
php 20.16% <ø> (ø)
python_submitty_utils 71.65% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

@williamjallen williamjallen self-requested a review March 2, 2022 21:53
@DefinitiveAbove DefinitiveAbove marked this pull request as draft March 21, 2022 21:57
Comment on lines 19 to 22

afterEach(() => {
cy.logout(true);
});
Copy link
Member

Choose a reason for hiding this comment

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

What happens if someone creates a test which doesn't log in, or logs out as part of the test? Would this fail if the test doesn't end in a logged-in state? Additionally, does leaving sessions hanging cause any issues? If not, do we need to be this explicit in making sure that each and every test logs out properly?

Copy link
Member Author

Choose a reason for hiding this comment

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

Leaving tests in a logged-in state works most of the time but can occasionally be flaky; see #7695 (comment) for more details.

I added in a list that lets certain tests be excluded from logging out, though it only excludes based on the full path of the current test title and not the file name itself (Cypress.spec reports differently when running all tests versus running each test individually so I don't think there's a way to check the filename without a hacky workaround, and I don't think there will be a case where there will be two tests with the same title paths).

Copy link
Member

Choose a reason for hiding this comment

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

Could we make the logout function have a conditional (activated via parameter) such that it can check if we're logged in first before triggering the action, else do nothing? I think that might be a bit cleaner than using text ignores that could be easy to inadvertently break.

@DefinitiveAbove DefinitiveAbove marked this pull request as ready for review March 23, 2022 07:41
Copy link
Member

@shailpatels shailpatels left a comment

Choose a reason for hiding this comment

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

Thanks for this, reran the e2e tests a couple times and the cypress portion seems stable.

The manual login and logout were previously added because there was an issue where the existing user would remain logged in between tests - this is supposed to be prevented by cypress clearing the cookie/session state between each one. Maybe something was fixed in either cypress or github actions or submitty? Either way good to know its working and should save time

@bmcutler
Copy link
Member

@DefinitiveAbove
Please resolve merge conflicts

@bmcutler bmcutler merged commit b679775 into master Apr 29, 2022
@bmcutler bmcutler deleted the cypress-intercept-xhr branch April 29, 2022 22:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants