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

Fix flaky test by clearing all BaseUrl cookies: 164803621 #1971

Merged
merged 12 commits into from Apr 8, 2019

Conversation

3 participants
@lynzt
Copy link
Contributor

lynzt commented Apr 5, 2019

Description

Fix flaky failing e2e tests: "expecting to find" button: https://www.pivotaltracker.com/story/show/164803621

Reviewer Notes

Our e2e tests are very flaky and keep failing. We tracked this down to the app trying to hit the login page, but we're still logged in so the app doesn't find the local user sign in button because we see the queues page.

This is due to cypress not clearing cookies when setting multiple BaseUrl. This calls cy.clearCookies() for each BaseUrl which should clear up any lingering cookies that keep us logged in.

Setup

Should pass CircleCI e2e tests...

Code Review Verification Steps

  • Request review from a member of a different team.
  • Have the Pivotal acceptance criteria been met for this change?

References

@lynzt lynzt added the ttv label Apr 5, 2019

}
csrfToken := csrfCookie.Value
// Generate a CSRF token
csrfToken := csrf.Token(r)

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Apr 5, 2019

Contributor

fwiw - I changed this previously to grab from the set cookie so we could debug this easier. If its causing an issue then I'd like to know the root issue with the middleware.

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Apr 5, 2019

Contributor

Also, I'm trying to get us away from using this page at all for e2e tests. See this PR: #1905

This comment has been minimized.

Copy link
@stangah

stangah Apr 5, 2019

Contributor

The main issue is that we were unnecessarily logging out (cookies are auto-cleared after every test), so then the first action in most tests becomes hitting this route which then fails because we haven't set the masked cookie yet but we're trying to read it. I think the choices are then:

  • Hit a dummy route first to set the masked cookie
  • Directly use the value, as above

Hitting a dummy route seemed... superfluous? But if the debugging gain is worth it I don't think it really impacts anything. Could hit the health route or something to keep it simple

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Apr 8, 2019

Contributor

@lynzt - Let's remove this change from this PR. If we need the gorilla cookie we can always hit / instead of the login page. And in #1905 we'll no longer hit this page at all, so the benefit of changing this is not super clear right now.

Fwiw, I'm open to the change, I'd just prefer to see it not in this PR.

@lynzt lynzt force-pushed the lt-ps-cookies branch from c5e2fc2 to 30cb87f Apr 6, 2019

@lynzt lynzt force-pushed the lt-ps-cookies branch from 48172e5 to f2e5d16 Apr 6, 2019

cy.signInAsUser('6cd03e5b-bee8-4e97-a340-fecb8f3d5465');
cy.waitForReactTableLoad();
});
Cypress.Commands.add('signInAsUser', userId => {
// make sure we log out first before sign in
cy.logout();

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Apr 8, 2019

Contributor

Does removing this help? I'd prefer we didn't guess that previous tests logged themselves out but instead guarantee that we always logout first. I'm pretty sure we added this previously because tests were not previously cleaning up their login state.

This comment has been minimized.

Copy link
@lynzt

lynzt Apr 8, 2019

Author Contributor

logout wasn't running as expected and we still had lingering cookies that allowed the test to still be logged in. Now that we're clearing cookies from all 3 BaseUrl, this seems like a redundant step that's no longer needed.

Would like to add a todo: to ensure we're testing the logout explicitly since we can remove this from running for all the e2e tests.

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Apr 8, 2019

Contributor

I guess that's ok but I'd prefer cy.setBaseUrlAndClearAllCookies(appName); be called here instead of in the other methods. The problem as I see it is that you're relying on people to know to call cy.setBaseUrlAndClearAllCookies(appName); before they call cy.signInAsUser(userId). I'd prefer not to assume people are going to do this. So you're right, cy.logout() was doing this with an attempt to clear cookies. So if we're now clearing cookies explicitly I'd prefer to replace it, not remove it.

This comment has been minimized.

Copy link
@lynzt

lynzt Apr 8, 2019

Author Contributor

very valid point! I will move add setBaseUrlAndClearAllCookies to the signInAsUser function...

@chrisgilmerproj
Copy link
Contributor

chrisgilmerproj left a comment

Overall really happy with this change. Thanks for fixing our tests.

I've got a couple minor fixes and then happy to approve.

@lynzt lynzt marked this pull request as ready for review Apr 8, 2019

@lynzt

This comment has been minimized.

Copy link
Contributor Author

lynzt commented Apr 8, 2019

@chrisgilmerproj

  • leaving out logout since it is redundant now that we clear cookies properly...
  • had to update some mymove test files to use signIntoMyMoveAsUser and not signInAsUser to have the flow go thru setting the baseUrl and clearing cookies...

@lynzt lynzt changed the title WIP: try and fix flaky test by fixing login cookies Fix flaky test by clearing all BaseUrl cookies: 164803621 Apr 8, 2019

@lynzt lynzt merged commit a688375 into master Apr 8, 2019

19 checks passed

Codacy/PR Quality Review Up to standards. A positive pull request.
Details
ci/circleci: acceptance_tests_experimental Your tests passed on CircleCI!
Details
ci/circleci: acceptance_tests_local Your tests passed on CircleCI!
Details
ci/circleci: acceptance_tests_staging Your tests passed on CircleCI!
Details
ci/circleci: build_app Your tests passed on CircleCI!
Details
ci/circleci: build_migrations Your tests passed on CircleCI!
Details
ci/circleci: build_tools Your tests passed on CircleCI!
Details
ci/circleci: client_test Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_api Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_mymove Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_office Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_tsp Your tests passed on CircleCI!
Details
ci/circleci: pre_deps_golang Your tests passed on CircleCI!
Details
ci/circleci: pre_deps_yarn Your tests passed on CircleCI!
Details
ci/circleci: pre_test Your tests passed on CircleCI!
Details
ci/circleci: server_test Your tests passed on CircleCI!
Details
ci/circleci: server_test_coverage Your tests passed on CircleCI!
Details
codecov/patch Coverage not affected when comparing 9b25a58...bee57a3
Details
codecov/project/go 60.51% remains the same compared to 9b25a58
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.