• 
      

    Fix unit tests with registry-backed SCMTools.

    Review Request #12390 — Created June 21, 2022 and submitted

    Information

    Review Board
    release-5.0.x

    Reviewers

    We hit some confusing issues when running selective Review Board test
    suites with the introduction of registry-backed SCMTools.

    Basically, when exclusively running certain test suites (ones that do
    not use the test_scmtools fixture on the class level, but do add it
    for selective tests through the use of @add_fixtures -- for instance,
    ./reviewboard/webapi/tests/test_review.py), we'd end up in a state
    where the fixture would try to apply on top of tools injected into the
    database through the registry. This is actually more the "correct"
    behavior, as opposed to test_scmtools applying on top of nothing, but
    it's the case that broke tests.

    The problem has to do with registry setup, when the registry adds to the
    database, and when fixtures load. The process is this:

    1. The first thing a test suite does is call Django's setUpClass(),
      then load class-defined fixtures, then call _fixture_setup() (which
      we override to compile and load fixtures -- too late and redundant on
      modern Django), and then setUp().

    2. Any test_scmtools fixtures are injected in Django's setUpClass().
      This gives us a common database entries used by all tests in the
      suite (using transactions/rollbacks to get back to that state). If
      that fixture is not used, no entries will be in the database for
      these tests.

    3. We register some test tools in our own setUpClass(), which triggers
      registry population. If test_scmtools was used, this does
      effectively nothing (though it will register entrypoint-provided
      tools). If it was not used, then this will inject its own common
      Tools entries in the database.

    4. We then get to our _fixture_setup(), which used to be where Django
      managed fixtures on a per-test basis. Djblets overrides this to do
      fixture compilation and loading (which doesn't work as well with
      modern Django) and loading of anything provided by @add_fixtures().
      Once again, any fixtures are loaded and injected into the database.
      If test_scmtools is used here, this is almost a no-op, as
      loaddata (which loads the fixtures) will ignore any identical
      matches in the database. If test_scmtools is not used on the class
      level, nothing happens here.

    5. Now we get to @add_fixtures(). This will try to incrementally add
      the fixtures to the database. If used on a test function, the
      _fixture_setup() for that call will load in any fixtures provided.
      If we use test_scmtools here and on the class level, nothing
      happens, since loaddata will see the data is the same. However, if
      the initial state was populated by the registry, this will fail, as
      the data won't match up. It'll conflict.

    There's a bit of a chicken-and-egg problem here. Confusing, but the
    solution is to treat our registry as the source of the fixture:

    1. The test_scmtools fixture is now empty. The fixture itself must
      remain for compatibility reasons, but it no longer needs content.
      It's simply an identifier.

    2. The SCMTools registry has a new function for the database population,
      called populate_db(). This is still called by populate(), but
      we're able to use it to force a re-sync of tools.

    3. TestCase.load_fixtures() (called by _fixture_setup()) now looks
      for test_scmtools, removes it from consideration, and then tells
      the registry to populate via populate_db(). This gives us one
      source of truth, and one place (setUpClass()) to manage suite-wide
      population of registered SCMTools.

    All unit tests passed.

    Ran selective unit test suites and individual tests that were previously
    failing. They now pass.

    Summary ID
    Fix unit tests with registry-backed SCMTools.
    We hit some confusing issues when running selective Review Board test suites with the introduction of registry-backed SCMTools. Basically, when exclusively running certain test suites (ones that do not use the `test_scmtools` fixture on the class level, but do add it for selective tests through the use of `@add_fixtures` -- for instance, `./reviewboard/webapi/tests/test_review.py`), we'd end up in a state where the fixture would try to apply on top of tools injected into the database through the registry. This is actually more the "correct" behavior, as opposed to `test_scmtools` applying on top of nothing, but it's the case that broke tests. The problem has to do with registry setup, when the registry adds to the database, and when fixtures load. The process is this: 1. The first thing a test suite does is call Django's `setUpClass()`, then load class-defined fixtures, then call `_fixture_setup()` (which we override to compile and load fixtures -- too late and redundant on modern Django), and then `setUp()`. 2. Any `test_scmtools` fixtures are injected in Django's `setUpClass()`. This gives us a common database entries used by all tests in the suite (using transactions/rollbacks to get back to that state). If that fixture is not used, no entries will be in the database for these tests. 3. We register some test tools in our own `setUpClass()`, which triggers registry population. If `test_scmtools` was used, this does effectively nothing (though it will register entrypoint-provided tools). If it was not used, then this will inject its own common `Tools` entries in the database. 4. We then get to our `_fixture_setup()`, which used to be where Django managed fixtures on a per-test basis. Djblets overrides this to do fixture compilation and loading (which doesn't work as well with modern Django) and loading of anything provided by `@add_fixtures()`. Once again, any fixtures are loaded and injected into the database. If `test_scmtools` is used here, this is almost a no-op, as `loaddata` (which loads the fixtures) will ignore any identical matches in the database. If `test_scmtools` is not used on the class level, nothing happens here. So far, things are fine. The registry, `setUpClass()`, and `_fixture_setup()` play nice. 5. Now we get to `@add_fixtures()`. This will try to incrementally add the fixtures to the database. If used on a test function, the `_fixture_setup()` for that call will load in any fixtures provided. If we use `test_scmtools` here and on the class level, nothing happens, since `loaddata` will see the data is the same. However, if the initial state was populated by the registry, this will fail, as the data won't match up. It'll conflict. There's a bit of a chicken-and-egg problem here. Confusing, but the solution is to treat our registry as the source of the fixture: 1. The `test_scmtools` fixture is now empty. The fixture itself must remain for compatibility reasons, but it no longer needs content. It's simply an identifier. 2. The SCMTools registry has a new function for the database population, called `populate_db()`. This is still called by `populate()`, but we're able to use it to force a re-sync of tools. 3. `TestCase.load_fixtures()` (called by `_fixture_setup()`) now looks for `test_scmtools`, removes it from consideration, and then tells the registry to populate via `populate_db()`. This gives us one source of truth, and one place (`setUpClass()`) to manage suite-wide population of registered SCMTools.
    2814f8d9b02a689c491d86e7d11879e23d8f813b
    Description From Last Updated

    Should we put something in this file to say that this fixture was deprecated? Then again this is only used …

    maubinmaubin
    david
    1. Ship It!
    2. 
        
    maubin
    1. 
        
    2. Show all issues

      Should we put something in this file to say that this fixture was deprecated? Then again this is only used internally so not that big of a deal.

      1. We can't, I'm afraid. No comments in JSON.

      2. Ah right, I'm too used to using hjson

    3. 
        
    chipx86
    Review request changed
    Status:
    Completed
    Change Summary:
    Pushed to release-5.0.x (44791cf)