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

[FIX] tests: make --test-file great again #43267

Closed

Conversation

@Elkasitu
Copy link
Contributor

Elkasitu commented Jan 14, 2020

Sort of but not really, this commit fixes a special case in which
launching a --test-file of a file with at least two SavepointCases would
create a postgresql deadlock and it would be impossible to terminate the
Odoo process without sending a SIGKILL or waiting for the lock to
timeout.

This was introduced at #39368 and happens because of the way that
unittests unwraps suites, to keep it short, when it unwraps the custom
OdooSuite class internally, it ends up with a vanilla TestSuite with
which to run the different test cases, and since #39368 depends on the
overrides added to OdooSuite to function, the class cleanups are not
triggered at the end of a test class (rollback, cache cleanups, env
reset, registry reset, etc.).

The fix is to manually unwrap the suite of tests to keep OdooSuite as
the suite with which to call the tests, which was already done for
--test-enable (although for different reasons, --test-tags?) which is
why --test-enable didn't have any problems.

This commit also fixes a typo I found on the backport, which meant
classCleanups were not being executed if the setUpClass failed, but it
had no effect on classCleanups during tearDownClass.

Task-ID 2160398
Depends on #43135

@Elkasitu Elkasitu requested a review from xmo-odoo Jan 14, 2020
@Elkasitu

This comment has been minimized.

Copy link
Contributor Author

Elkasitu commented Jan 14, 2020

@xmo-odoo here's another reason to get rid of --test-file (or streamline the way we execute tests or use a better testing framework, etc.)

@robodoo robodoo added the seen 🙂 label Jan 14, 2020
@Elkasitu

This comment has been minimized.

Copy link
Contributor Author

Elkasitu commented Jan 14, 2020

@Whenrow This fixes the bug with --test-file and SavepointCase you brought to me some time ago, FYI

@robodoo robodoo added the CI 🤖 label Jan 14, 2020
@C3POdoo C3POdoo added the RD label Jan 14, 2020
@xmo-odoo

This comment has been minimized.

Copy link
Collaborator

xmo-odoo commented Jan 14, 2020

@Elkasitu is it normal that it includes / is based on #43135?

@Whenrow

This comment has been minimized.

Copy link
Contributor

Whenrow commented Jan 14, 2020

@Elkasitu Thanks a lot !

@Elkasitu

This comment has been minimized.

Copy link
Contributor Author

Elkasitu commented Jan 14, 2020

@xmo-odoo yeah I did it to avoid conflicts as I assumed your PR would be merged first, worst case I can just drop the commit

@Elkasitu Elkasitu force-pushed the odoo-dev:saas-13.1-fix-test-file-adt branch from 02a4873 to 67c277d Jan 14, 2020
@robodoo robodoo removed the CI 🤖 label Jan 14, 2020
Sort of but not really, this commit fixes a special case in which
launching a --test-file of a file with at least two SavepointCases would
create a postgresql deadlock and it would be impossible to terminate the
Odoo process without sending a SIGKILL or waiting for the lock to
timeout.

This was introduced at #39368 and happens because of the way that
unittests unwraps suites, to keep it short, when it unwraps the custom
OdooSuite class internally, it ends up with a vanilla TestSuite with
which to run the different test cases, and since #39368 depends on the
overrides added to OdooSuite to function, the class cleanups are not
triggered at the end of a test class (rollback, cache cleanups, env
reset, registry reset, etc.).

The fix is to manually unwrap the suite of tests to keep OdooSuite as
the suite with which to call the tests, which was already done for
--test-enable (although for different reasons, --test-tags?) which is
why --test-enable didn't have any problems.

This commit also fixes a typo I found on the backport, which meant
classCleanups were not being executed if the setUpClass failed, but it
had no effect on classCleanups during tearDownClass.

Task-ID 2160398
Depends on #43135
@Elkasitu Elkasitu force-pushed the odoo-dev:saas-13.1-fix-test-file-adt branch from 67c277d to 93a48d6 Jan 14, 2020
@robodoo robodoo added the CI 🤖 label Jan 14, 2020
@Elkasitu

This comment has been minimized.

Copy link
Contributor Author

Elkasitu commented Jan 14, 2020

robodoo r+

@robodoo robodoo added the r+ 👌 label Jan 14, 2020
robodoo pushed a commit that referenced this pull request Jan 14, 2020
Sort of but not really, this commit fixes a special case in which
launching a --test-file of a file with at least two SavepointCases would
create a postgresql deadlock and it would be impossible to terminate the
Odoo process without sending a SIGKILL or waiting for the lock to
timeout.

This was introduced at #39368 and happens because of the way that
unittests unwraps suites, to keep it short, when it unwraps the custom
OdooSuite class internally, it ends up with a vanilla TestSuite with
which to run the different test cases, and since #39368 depends on the
overrides added to OdooSuite to function, the class cleanups are not
triggered at the end of a test class (rollback, cache cleanups, env
reset, registry reset, etc.).

The fix is to manually unwrap the suite of tests to keep OdooSuite as
the suite with which to call the tests, which was already done for
--test-enable (although for different reasons, --test-tags?) which is
why --test-enable didn't have any problems.

This commit also fixes a typo I found on the backport, which meant
classCleanups were not being executed if the setUpClass failed, but it
had no effect on classCleanups during tearDownClass.

Task-ID 2160398
Depends on #43135

closes #43267

Signed-off-by: Adrian Torres (adt) <adt@odoo.com>
@robodoo robodoo closed this Jan 14, 2020
@robodoo robodoo deployed to merge Jan 14, 2020 Active
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.