Fix unit tests with registry-backed SCMTools.
Review Request #12390 — Created June 20, 2022 and submitted
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_scmtoolsfixture 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_scmtoolsapplying 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:
The first thing a test suite does is call Django's
then load class-defined fixtures, then call
we override to compile and load fixtures -- too late and redundant on
modern Django), and then
test_scmtoolsfixtures are injected in Django's
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
We register some test tools in our own
setUpClass(), which triggers
registry population. If
test_scmtoolswas used, this does
effectively nothing (though it will register entrypoint-provided
tools). If it was not used, then this will inject its own common
Toolsentries in the database.
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
Once again, any fixtures are loaded and injected into the database.
test_scmtoolsis used here, this is almost a no-op, as
loaddata(which loads the fixtures) will ignore any identical
matches in the database. If
test_scmtoolsis not used on the class
level, nothing happens here.
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_scmtoolshere and on the class level, nothing
loaddatawill 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:
test_scmtoolsfixture is now empty. The fixture itself must
remain for compatibility reasons, but it no longer needs content.
It's simply an identifier.
The SCMTools registry has a new function for the database population,
populate_db(). This is still called by
we're able to use it to force a re-sync of tools.
_fixture_setup()) now looks
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.
Should we put something in this file to say that this fixture was deprecated? Then again this is only used …
reviewboard/scmtools/fixtures/test_scmtools.json (Diff revision 1)
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.