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

Remove resetDB() in cypress tests in favor of `make db_e2e_reset` being called manually #960

Merged
merged 4 commits into from Sep 7, 2018

Conversation

2 participants
@chrisgilmerproj
Contributor

chrisgilmerproj commented Sep 5, 2018

Description

A lot of our tests have cy.resetDb(); attached to the last test. This seems brittle since the last test may change or the order might change. Instead use the after() hook from cypress to call this function instead.

Reviewer Notes

I've added this after() clause to all our tests. This may not be necessary but I like being consistent.

Setup

make e2e_test

Code Review Verification Steps

  • End to end tests pass (make e2e_test).
  • Request review from a member of a different team.

References

@chrisgilmerproj chrisgilmerproj self-assigned this Sep 5, 2018

@tinyels

This comment has been minimized.

Show comment
Hide comment
@tinyels

tinyels Sep 5, 2018

Contributor

This is considered a bad practice in Cypress and will really slow the tests down.

Contributor

tinyels commented Sep 5, 2018

This is considered a bad practice in Cypress and will really slow the tests down.

@tinyels

This is exactly the opposite of what the Cypress best practices recommend. https://docs.cypress.io/guides/references/best-practices.html#It%E2%80%99s-all-downside-with-no-upside

@chrisgilmerproj

This comment has been minimized.

Show comment
Hide comment
@chrisgilmerproj

chrisgilmerproj Sep 5, 2018

Contributor

Aha! I'm glad you brought this up, I had noticed the tests slow a bit. But I really dislike the idea of having it attached to the last test. Perhaps we can make a test that is like this to be more clear:

it('LAST STEP resets DB', function() {
  assert.isTrue(true, 'this val is true')
  cy.resetDb();
});
Contributor

chrisgilmerproj commented Sep 5, 2018

Aha! I'm glad you brought this up, I had noticed the tests slow a bit. But I really dislike the idea of having it attached to the last test. Perhaps we can make a test that is like this to be more clear:

it('LAST STEP resets DB', function() {
  assert.isTrue(true, 'this val is true')
  cy.resetDb();
});
@chrisgilmerproj

This comment has been minimized.

Show comment
Hide comment
@chrisgilmerproj

chrisgilmerproj Sep 5, 2018

Contributor

Oh, also in reading those docs it says if we really want a DB reset we ought to do it in the before() hook: https://docs.cypress.io/guides/references/best-practices.html#State-reset-should-go-before-each-test

Contributor

chrisgilmerproj commented Sep 5, 2018

Oh, also in reading those docs it says if we really want a DB reset we ought to do it in the before() hook: https://docs.cypress.io/guides/references/best-practices.html#State-reset-should-go-before-each-test

@chrisgilmerproj chrisgilmerproj changed the title from Use after() in cypress tests to reset db after each set of tests to Use before() in cypress tests to reset db before each set of tests Sep 5, 2018

@chrisgilmerproj

This comment has been minimized.

Show comment
Hide comment
@chrisgilmerproj

chrisgilmerproj Sep 6, 2018

Contributor

I'm going to attempt to remove all the cy.resetDB() calls and see if the tests run. And then follow up with seeing how the tests work if you call make db_e2e_reset occasionally while doing test development.

Contributor

chrisgilmerproj commented Sep 6, 2018

I'm going to attempt to remove all the cy.resetDB() calls and see if the tests run. And then follow up with seeing how the tests work if you call make db_e2e_reset occasionally while doing test development.

* .exec('make db_e2e_reset')
* .its('code')
* .should('eq', 0),
*/

This comment has been minimized.

@chrisgilmerproj

chrisgilmerproj Sep 6, 2018

Contributor

I considered adding a failure here but felt like it'd be easier to just leave this as an empty function with comments in it.

@chrisgilmerproj

chrisgilmerproj Sep 6, 2018

Contributor

I considered adding a failure here but felt like it'd be easier to just leave this as an empty function with comments in it.

@chrisgilmerproj

This comment has been minimized.

Show comment
Hide comment
@chrisgilmerproj

chrisgilmerproj Sep 6, 2018

Contributor

The e2e tests all pass with this change. I also was able to run make db_e2e_reset while modifying tests and saw the changes I wanted. I think this is an appropriate change going forward.

Contributor

chrisgilmerproj commented Sep 6, 2018

The e2e tests all pass with this change. I also was able to run make db_e2e_reset while modifying tests and saw the changes I wanted. I think this is an appropriate change going forward.

@chrisgilmerproj chrisgilmerproj changed the title from Use before() in cypress tests to reset db before each set of tests to Remove resetDB() in cypress tests in favor of `make db_e2e_reset` being called manually Sep 6, 2018

@tinyels

tinyels approved these changes Sep 7, 2018

@tinyels

This comment has been minimized.

Show comment
Hide comment
@tinyels

tinyels Sep 7, 2018

Contributor

This would be a good thing to talk about at Dod All Together!

Contributor

tinyels commented Sep 7, 2018

This would be a good thing to talk about at Dod All Together!

@chrisgilmerproj

This comment has been minimized.

Show comment
Hide comment
@chrisgilmerproj

chrisgilmerproj Sep 7, 2018

Contributor

Good idea! I'll try to bring it up today.

Contributor

chrisgilmerproj commented Sep 7, 2018

Good idea! I'll try to bring it up today.

@chrisgilmerproj chrisgilmerproj merged commit a4bca20 into master Sep 7, 2018

3 checks passed

ci/circleci: build_app Your tests passed on CircleCI!
Details
ci/circleci: build_migrations Your tests passed on CircleCI!
Details
ci/circleci: pre_test Your tests passed on CircleCI!
Details

@chrisgilmerproj chrisgilmerproj deleted the cg_reset_db_after_e2e_tests branch Sep 7, 2018

@chrisgilmerproj chrisgilmerproj referenced this pull request Sep 12, 2018

Merged

Adding an e2e test to cancel moves #978

3 of 3 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment