[FIX] tests: @users must cleanup between users

[odony]

The @users decorator introduced via #21499 executes the test function once for every user specified, but does not perform the cleanup steps that are expected for TransactionCase test methods.

This can cause side-effects and false positives during tests that are sensitive to the presence of prior test data, as shown by the example test added by this commit (which required a small ACL change, as non-admin users need access to the test model)

Notes:

• This could possibly be backported to 12.0, though existing tests have managed to workaround this bug so far, and new ones are more likely to be written in master.
• The cleanup code is purposely dumb, without the precautions that the unittest runner normally takes (e.g. when setUp raises, doCleanups is still supposed to be invoked). We don't want to re-implement the full unittest stack here. If more niceties are necessary, it's probably a sign we should implement @users in a different manner, and better integrate with unittest (e,g. via preprocessing or something)

[xmo-odoo]

Looks OK to me, though I don't know that this needs to be within the subtest (setup/teardown/cleanup normally run outside the test itself), and according to TestCase.run the cleanups run after the teardown.

Furthermore we probably need to consider what's supposed to (and what's going to) happen if either teardown or cleanups fail.

[odony]

Looks OK to me, though I don't know that this needs to be within the subtest (setup/teardown/cleanup normally run outside the test itself)

Good point! For the reasons mentioned in the description I did not want to explicitly handle the cases where the teardown/cleanup/setup raises, so keeping it within subTest() would make little difference.

and according to TestCase.run the cleanups run after the teardown.

Isn't that what the patch is doing too?

Furthermore we probably need to consider what's supposed to (and what's going to) happen if either teardown or cleanups fail.

I'd say we'll crash the whole test run horribly, and leave a barren wasteland with smoking bits here and there. But really, do we want to duplicate the whole testrunner logic in our own code? Isn't that a clue that we're doing the whole @users thing incorrectly?

[xmo-odoo]

Isn't that what the patch is doing too?

Oh yeah, for some reason I'd convinced myself it was the other way around.

I'd say we'll crash the whole test run horribly, and leave a barren wasteland with smoking bits here and there. But really, do we want to duplicate the whole testrunner logic in our own code? Isn't that a clue that we're doing the whole @users thing incorrectly?

Nah, what I'm thinking however is that the test runner will re-run something which may already have run, trying its own hand at teardown-ing the test, and I'm not sure how understandable it'll look.

[pedrobaeza]

Coming to this through codetriage.com. @odony is this still applicable?

[xmo-odoo]

@odony found this when browsing through my old reviews and figured a "cleaner" version in the sense that it wouldn't require hand-rolling and chasing the test protocol (e.g. supporting addCleanup and friends): unittest uses dir() to discover test methods, and since Python 3 that can be overridden (using __dir__), so we could just generate "virtual" tests per-login e.g. test_cleanup[admin], and as long as __getattr__ is overridden to match as far as unittest is concerned that's a completely normal test.

Then we can just wrap the original test function in one setting the uid before running the wrappee (with an addCleanup to reset the uid afterwards) and we're good, I think.