Improve test isolation and reduce test verbosity #7186 #10710

[jacobtylerwalls]

Types of changes

• Bugfix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)

Description of Change

A few things make the test suite harder to work with when developing new tests, reviewing them, or debugging failing ones:

• tests that can't be run in isolation because they depend on other graph or ontology loads
• tests that fail to execute their setup code with cryptic errors like this when other tests are trying to rollback transactions:
  -- psycopg2.errors.ObjectInUse: cannot TRUNCATE "geojson_geometries" because it has pending trigger events
• tests that dump output to the console (harder to pick out errors, or to avoid contributing to the problem further)

Now, the tests run quietly, so that failures can be seen at a glance. Also, the suite runs about twice as fast.

Found 323 test(s).
Creating test database for alias 'default'...
added admin group
added users group
creating index : test_terms
creating index : test_concepts
creating index : test_resources
System check identified no issues (2 silenced).
................................................................................................................
................................................................................................................
...................................................................................................
----------------------------------------------------------------------
Ran 323 tests in 184.422s

OK
deleting index : test_terms
deleting index : test_concepts
deleting index : test_resources
Destroying test database for alias 'default'...

As part of this work, I noticed #10710 and solved it in an individual commit on this branch and added a release note.

Issues Solved

Closes #7186
Closes #10710

Checklist

• Unit tests pass locally with my changes
• I have added tests that prove my fix is effective or that my feature works
• I have added necessary documentation (if appropriate)

Ticket Background

• Sponsored by: Farallon
• Found by: @
• Tested by: @jacobtylerwalls
• Designed by: @

Further comments

To avoid contributing to this in the future, a couple things are worth noting:

• When testing a request that returns 4xx or 5xx errors, we can use self.assertLogs(...) to capture the logged output and assert against it
• When running commands that print to the console (instead of logging) e.g. load package commands, we can use the django util with captured_stdout():.

[apeters]

Is there a reason why this is in the "icebox". This seems like a nice improvement. This should be put into the QA pipeline as far as I'm concerned.

[jacobtylerwalls]

No reason, just need to find a reviewer. Can you take a look @apeters?

[apeters]

when I try and run tests locally, I get errors related to webpack.stats

OSError: Error reading /Users/alexei/Work/Projects/arches/shpo/arches/arches/webpack/webpack-stats.json. 
Are you sure webpack has generated the file and the path is correct?

[jacobtylerwalls]

Yeah, you have to build yarn in the arches directory you're running the tests against. I'll update arches-docs with that. Let me know if that doesn't resolve it.

EDIT: both yarn install && yarn start

[jacobtylerwalls]

Will hold off on proposing a docs update, because an alternative would be just adding the missing file to version control (and leave it git-ignored in the project templates).

The file just looks like this, give or take whether or not somebody has local_settings fiddling with STATIC_URL:

{
  "status": "compile",
  "assets": {},
  "chunks": {},
  "publicPath": "/static/"
}

[apeters]

any reason we removed the tearDownClass method?

[apeters]

any reason we removed the tearDownClass method?

[apeters]

any reason we removed the tearDownClass method?

[apeters]

shouldn't we still be deleting this somewhere?

[jacobtylerwalls]

Yeah, good Q!

Django wraps test cases in transactions, so you don't need to undo the changes you make to the test db -- each one just rolls back to the savepoint.

(You might still need to run teardown code if you have other python changes (to module constants, settings, etc.), but these deleted teardown methods weren't doing that.)

So at best, they're just cruft, but at worst, they can cause trouble:

• if you forget to call super().tearDownClass(), which is when Django cleans up the transaction, the next test case class can throw cryptic failures. I had to look into this in the controlled list manager, and I think Christopher saw something similar that I chose not to address in this PR because it was too hard to fix. Most of the missing super() behavior was fixed in Add missing super() calls #10525 #10530. I opened Ensure all package loads in setUpClass() take place inside a transaction #10719 for the followup.
• it's possible the teardown code won't work if the transaction is in the middle of aborting (because the test failed, or because the test tested a code path that involves a transaction aborting, like handling validation errors). It would probably error out with cryptic stuff about "pending trigger events".

[apeters]

Yeah, good Q!

Django wraps test cases in transactions, so you don't need to undo the changes you make to the test db -- each one just rolls back to the savepoint.

(You might still need to run teardown code if you have other python changes (to module constants, settings, etc.), but these deleted teardown methods weren't doing that.)

So at best, they're just cruft, but at worst, they can cause trouble:

• if you forget to call super().tearDownClass(), which is when Django cleans up the transaction, the next test case class can throw cryptic failures. I had to look into this in the controlled list manager, and I think Christopher saw something similar that I chose not to address in this PR because it was too hard to fix. Most of the missing super() behavior was fixed in Add missing super() calls #10525 #10530. I opened Ensure all package loads in setUpClass() take place inside a transaction #10719 for the followup.
• it's possible the teardown code won't work if the transaction is in the middle of aborting (because the test failed, or because the test tested a code path that involves a transaction aborting, like handling validation errors). It would probably error out with cryptic stuff about "pending trigger events".

Thanks for the very complete description of the issues and solutions. I guess I never realized that TestCase inherited from TransactionTestCase. That would have been very helpful to know when I was making new test classes a while back.

[apeters]

I still can't get the tests to complete fully. It seems to hang at some point and not finish, but that seems to be an issue for me without these improvements as well. I do see that the tests that do run (which are many), do run without any unnecessary output. Because of that I'll approve and try and figure out why I can't get tests to finish.