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: remove cookie check during logout because of flake [DET-3823] #1062

Merged
merged 1 commit into from
Aug 13, 2020

Conversation

hkang1
Copy link
Contributor

@hkang1 hkang1 commented Aug 12, 2020

Description

It seems like with cypress's whitelisting of the auth cookie, there is no guarantee that the cookie will be properly cleared under the testing environment. We currently have a url path check to confirm that the logout takes you to the sign in page, which should suffice. Simply removed the cookie check due to its flakiness.

Reference:
cypress-io/cypress#5723
cypress-io/cypress#5453

Test Plan

Commentary (optional)

@hkang1 hkang1 requested a review from hamidzr August 12, 2020 03:53
@cla-bot cla-bot bot added the cla-signed label Aug 12, 2020
@hkang1 hkang1 changed the title fix: remove cookie check due to flake introduced in cypress fix: remove cookie check during logout because of flake [DET-3823] Aug 12, 2020
@hkang1 hkang1 assigned hkang1 and unassigned hamidzr Aug 12, 2020
@hkang1 hkang1 force-pushed the 3823-fix-logout-test-flake branch 5 times, most recently from f6af26c to f452cca Compare August 13, 2020 05:04
@hkang1 hkang1 assigned hamidzr and unassigned hkang1 Aug 13, 2020
@hkang1
Copy link
Contributor Author

hkang1 commented Aug 13, 2020

@hamidzr I've reran the e2e-tests multiple times and I think it's stable now. About 6~10 recent reruns in a row have passed. 🤞

@hkang1
Copy link
Contributor Author

hkang1 commented Aug 13, 2020

The most recent push change is around dropping unused comment lines.

@hamidzr
Copy link
Member

hamidzr commented Aug 13, 2020

Description

It seems like with cypress's whitelisting of the auth cookie, there is no guarantee that the cookie will be properly cleared under the testing environment. We currently have a url path check to confirm that the logout takes you to the sign in page, which should suffice. Simply removed the cookie check due to its flakiness.

Reference:
cypress-io/cypress#5723
cypress-io/cypress#5453

Test Plan

Commentary (optional)

I looked to see if we can test our test helpers but I don't know how to set it up to expect some tests or a specific cypress command to fail. there are assertions but I didn't see a clean way of using it.
jestjs/jest#10030

describe('Sign in/out test tests', () => {
  it('should login and checkLoggedIn', () => {
    cy.login();
    cy.checkLoggedIn();
  });

  it('should logout and checkLoggedOUt', () => {
    cy.logout();
    cy.checkLoggedOut();
  });

  it('should logout and fail checkLoggedIn', () => {
    cy.logout();
    cy.checkLoggedIn()
    // expect(cy.checkLoggedIn).to.throw();
  });

  it('should login and fail checkLoggedOut', () => {
    cy.login();
    cy.checkLoggedOut()
    expect(cy.checkLoggedOut).to.throw();
  });
});

Copy link
Member

@hamidzr hamidzr left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for fixing this

@hamidzr hamidzr assigned hkang1 and unassigned hamidzr Aug 13, 2020
@hkang1 hkang1 merged commit 5982751 into determined-ai:master Aug 13, 2020
@hkang1 hkang1 deleted the 3823-fix-logout-test-flake branch August 13, 2020 19:35
@dannysauer dannysauer added this to the 0.13.0 milestone Feb 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants