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

Refactor System Tests to Eliminate Order-Dependence #1286

Closed
mbarnett opened this issue Sep 9, 2019 · 1 comment
Closed

Refactor System Tests to Eliminate Order-Dependence #1286

mbarnett opened this issue Sep 9, 2019 · 1 comment

Comments

@mbarnett
Copy link
Contributor

mbarnett commented Sep 9, 2019

The Problem

The system test cases have been flapping for a while, and this is made much worse by #1273 -- this is being caused by the way we load ActiveRecord fixtures once at the beginning of a system test run, and the way that ActionDispatch::SystemTestCase doesn't truncate data from tables at the end of any given test file. Both of these behaviours interact poorly with the way we system tests are structured, eg) CollectionsPaginationAndSortTest creates 11 collections and then assumes those are the only 11 in the database.

This both causes the test to be unreliable (sometimes, some other test that creates a collection will have run before it, invalidating the assertion on line 19) AND creates unreliability for other tests (none of the 11 collections created here are deleted, so all hang around and can potentially invalidate assertions in tests that run after this).

On the flip side, if we were to manually run database cleaner in the setup phase of each file, this breaks other system tests like DepositItemTest, which relies on fixtures. Because fixtures are loaded only once at the beginning of all tests, if the test database data has been deleted prior to this test running (in effect, if this test file is not the first one run), this call to load a fixture will error out.

This can only be resolved be fixing the inconsistencies with our approach to using data in system tests. Likely the best approach is to let everything that can be loaded in fixtures be loaded that way, to write the tests in such a way as to minimize both the creation of new objects and to minimize assumptions made about those being the only ones present, and wherever appropriate, to have tests clean up after themselves.

Short-term

If we're not going to remediate this immediately, we need to disable system tests on CI. An unreliable test is worse than a useless one.

@mbarnett
Copy link
Contributor Author

The remaining work on this is being tracked in a spike in clubhouse. Connor improved the situation with his PR, but there's still some flakiness in a couple of spots.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants