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

Widget leak detector during JS unit tests #30461

Closed
wants to merge 1 commit into from

Conversation

xmo-odoo
Copy link
Collaborator

Widgets have to be explicitly destroyed in unit tests. Non-trivial widgets which are not properly destroyed can lead to odd issues and failings in following tests (e.g. because they're intercepting events, have timers running, or are otherwise interfering with the widgets actually being tested).

This PR implements a "leak checker" of sorts: it records all the widgets created during the test, and fails if some of them are un-destroyed at the end. It also fixes extant instances of that issue.

Note that this will not catch all such leak checks: if a widget is created in an async callback and the test is not marked async (aka it's created after the tests has ended as far as qunit is concerned) it won't be detected.

It also doesn't handled the path issue: most tests only properly cleanup and terminate their tests in case no error happened (a deferred of some sort resolved correctly), this can cause cascading errors where one test failing would either lead to its timeout or interfere with further tests.

@C3POdoo C3POdoo added the RD research & development, internal work label Jan 23, 2019
@xmo-odoo
Copy link
Collaborator Author

@aab-odoo should I submit the fixes as individual PRs for you or @ged-odoo to approve, or should I just merge them myself?

There also seems to be a recurring issue I'm not sure how to fix (just hide it maybe?): services are parented & get created by the mock thing, but they never get destroyed, and I don't know that there's a way to get a handle on them to destroy them explicitly (plus it would be inconvenient?)

@aab-odoo
Copy link
Contributor

@xmo-odoo Services can be destroyed as they uses the ParentedMixin. Actually, they should be destroyed in addMockEnvironment (in the destroy override).

@aab-odoo
Copy link
Contributor

@xmo-odoo We can review it. Just tell us when you're done.

@xmo-odoo
Copy link
Collaborator Author

@xmo-odoo We can review it. Just tell us when you're done.

I was thinking the individual fixes being backported rather than this one, I'll probably be cherry-picking the fixes for the issues I found (e.g. missing super calls) to the relevant branch before this one is done.

@aab-odoo
Copy link
Contributor

@xmo-odoo I agree, but we can still review each PR

@xmo-odoo
Copy link
Collaborator Author

xmo-odoo commented Jan 29, 2019

🆗

Log an error if widgets created during a test are left undestroyed at
its end.

Note: ignore RamStorage because mail's testutils creates tons of them
and provides no way to clean them up.
@robodoo robodoo added the CI 🤖 Robodoo has seen passing statuses label Jun 25, 2020
@xmo-odoo
Copy link
Collaborator Author

xmo-odoo commented Jun 25, 2020

Well after updating to the latest master, this checker looks useless as it finds no leak whatsoever (in community anyway).

@xmo-odoo xmo-odoo closed this Jun 25, 2020
@robodoo robodoo added closed 💔 and removed CI 🤖 Robodoo has seen passing statuses labels Jun 25, 2020
@xmo-odoo xmo-odoo deleted the master-leakcheck-xmo branch June 25, 2020 11:04
@aab-odoo
Copy link
Contributor

Well after updating to the latest master, this checker looks useless as it finds no leak whatsoever (in community anyway).

💪

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RD research & development, internal work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants