Skip to content

[16.0][FIX] auditlog: Ensure unit tests unpatch methods. - #3364

Merged
OCA-git-bot merged 1 commit into
OCA:16.0from
amh-mw:16.0-test-contamination
Sep 24, 2025
Merged

OCA-git-bot merged 1 commit into
OCA:16.0from
amh-mw:16.0-test-contamination

Conversation

@amh-mw

@amh-mw amh-mw commented Sep 5, 2025

Copy link
Copy Markdown
Member

Though TransactionCase rolls back database changes, it does not call unlink on auditlog.rule records, meaning that unsubscribe is never called to unpatch model methods. This causes subsequent tests in other modules to unexpectedly execute patched methods.

This commit adds the AuditLogRuleCommon test class in an attempt to identify and prevent patched methods from contaminating the test environment.

@amh-mw

amh-mw commented Sep 5, 2025

Copy link
Copy Markdown
Member Author

@AungKoKoLin1997 Would you kindly review this pull request? It fixes #3224.

@AungKoKoLin1997 AungKoKoLin1997 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about like this?
This can reduce multiple lines.

Comment thread auditlog/tests/common.py
Comment thread auditlog/tests/test_auditlog.py
Comment thread auditlog/tests/test_auditlog.py
@amh-mw
amh-mw force-pushed the 16.0-test-contamination branch from 7685850 to 0fc48d8 Compare September 8, 2025 11:38

@AungKoKoLin1997 AungKoKoLin1997 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@yostashiro yostashiro left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks for the fix!

Comment thread auditlog/tests/common.py

@StefanRijnhart StefanRijnhart left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code looks like a welcome improvement to have auditlog tests clean up properly afterwards. But apart from that, is it not the case that the multicompany issue will still persist? Even though it is not triggered in the UI, will it still pop up when records are written in code?

@amh-mw

amh-mw commented Sep 15, 2025

Copy link
Copy Markdown
Member Author

is it not the case that the multicompany issue will still persist?

Though #3224 describes itself as a multi-company issue, standard Odoo demo data installed for testing already has multiple companies. I perceive it as a test environment contamination problem, not a multi-company problem. The docker-compose.test.yaml I added to that issue is my litmus for success; red before and green after this pull request.

@StefanRijnhart StefanRijnhart left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough, it's true that the issue mentions unit tests explicitely. I'll be fixing the multi-company issue in a separate PR.

Comment thread auditlog/tests/common.py Outdated
@StefanRijnhart

StefanRijnhart commented Sep 16, 2025

Copy link
Copy Markdown
Member

Picked into #3371 to fix the tests. There, the problem was indeed not the multi-company aspect fixed in the same PR but the fact that the patched methods mask method overrides loaded after the patching.

Though TransactionCase rolls back database changes, it does not
call `unlink` on `auditlog.rule` records, meaning that `unsubscribe`
is never called to unpatch model methods. This causes subsequent
tests in other modules to unexpectedly execute patched methods.

This commit adds the `AuditLogRuleCommon` test class in an
attempt to identify and prevent patched methods from contaminating
the test environment.
@amh-mw
amh-mw force-pushed the 16.0-test-contamination branch from e7c8308 to 8c2a4d1 Compare September 24, 2025 16:46

@StefanRijnhart StefanRijnhart left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the update!

/ocabot merge patch

@OCA-git-bot

Copy link
Copy Markdown
Contributor

What a great day to merge this nice PR. Let's do it!
Prepared branch 16.0-ocabot-merge-pr-3364-by-StefanRijnhart-bump-patch, awaiting test results.

@OCA-git-bot
OCA-git-bot merged commit d80965a into OCA:16.0 Sep 24, 2025
8 of 9 checks passed
@OCA-git-bot

Copy link
Copy Markdown
Contributor

Congratulations, your PR was merged at d0d5875. Thanks a lot for contributing to OCA. ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants