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

Verify post-test cleanup for model mapping unit tests; fix a few errors #12714

Closed

Conversation

jdavcs
Copy link
Member

@jdavcs jdavcs commented Oct 14, 2021

This fixes a few errors in main model and TS model mapping tests; and adds a test that verifies that such errors don't happen.

Model mapping tests (test/unit/shed_unit/model/test_mapping and test/unit/data/model/test_mapping) verify the correctness of the mapping specified in the model classes (more details in "Tests" section in the description of #12064). They use an in-memory sqlite database as a test double. The database is created once for each module (there are 2 modules (one for Galaxy, one for TS). Each test receives an empty database (with the tables created, but empty); it persists some objects, then cleans-up by removing anything it has added to the database.

However, it appears that in a few cases the cleanup was not properly handled (discovered thanks to a discussion with @mvdbeek in #12666 (review)). This PR fixes these errors and adds a fixture that auto-runs before each test and any unscoped fixtures (except session and model on which it depends) and verifies that all model tables in the database are empty. Thus, it ensures that a test is not affected by data leftover from a previous test run.

Limitations and tradeoffs

  1. The Galaxy model does not run the check for HistoryAudit and JobStateHistory models. These items are persisted indirectly (one is created by a database trigger, the other one - by a Job, upon Job instantiation). As a result, a test where either one ends up being added to the database has no way of knowing which HistoryAudit and/or JobStateHistory rows have been created (cleanup is handled by primary key: a test adding a Foo row (pkey=n) to the database, should delete only that Foo row (pkey=n) upon exit, not all Foo rows.). However, these two classes do not cause any test overlap issues, so I think this is a non-issue.
  2. Test execution time. The verification step executes N statements of the form SELECT count(*) FROM Foo
    where N = |models under test| * |tests| = 153 * 443 = 67,779. This increases the runtime by approx. 350% (on my desktop 4 sec. becomes 14 sec.). It's annoying, but negligible, I think.
  3. Some minor technical dept is added (fixture duplicated in 2 places), same as mentioned in TS model: declarative mapping + prep for SQLAlchemy 2.0 #12666

EDIT: the added test execution time is sufficiently annoying (even on my relatively fast desktop), so I'm wondering if this assertion is not too high a price to pay? It only asserts that the tests are setup correctly. If anyone feels the same, we could (a) remove the autouse=True - so it won't run - and add a comment that this can be enabled for test debugging if needed; or (b) remove it completely, and only leave the fixes.

How to test the changes?

(Select all options that apply)

  • I've included appropriate automated tests.
  • This is a refactoring of components with existing test coverage.
  • Instructions for manual testing are as follows:
    1. [add testing steps and prerequisites here if you didn't write automated tests covering all your changes]

License

  • I agree to license these contributions under Galaxy's current license.
  • I agree to allow the Galaxy committers to license these and all my past contributions to the core galaxy codebase under the MIT license. If this condition is an issue, uncheck and just let us know why with an e-mail to [email protected].

@jdavcs jdavcs added this to the 22.01 milestone Oct 14, 2021
@jdavcs jdavcs force-pushed the dev_mapping_tests_verification branch from 7a7a638 to cef3706 Compare October 14, 2021 21:50
@jdavcs jdavcs removed the status/WIP label Oct 14, 2021
@jdavcs jdavcs marked this pull request as ready for review October 14, 2021 23:17
@jdavcs jdavcs force-pushed the dev_mapping_tests_verification branch from cef3706 to 68426d2 Compare October 15, 2021 02:08
@jdavcs jdavcs force-pushed the dev_mapping_tests_verification branch from 68426d2 to b6a9d33 Compare October 15, 2021 02:09
@jdavcs
Copy link
Member Author

jdavcs commented Oct 22, 2021

@jmchilton thanks for reviewing! I have another PR coming up which is a restructuring of these mapping tests; it looks like a rebase from hell waiting to happen, so I'll close this and incorporate these changes into the updated files.

@jdavcs jdavcs closed this Oct 22, 2021
jdavcs added a commit to jdavcs/galaxy that referenced this pull request Oct 22, 2021
jdavcs added a commit to jdavcs/galaxy that referenced this pull request Oct 22, 2021
Disable test verification fixture by default (too expensive!)
Add comment explaining fixture + how to enable it.
@jdavcs jdavcs deleted the dev_mapping_tests_verification branch October 22, 2021 16:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants