search

iTwin / auth-clients

MIT License
6 stars 3 forks source link

`oidc-signin-tool`: Error: global leak(s) detected: '_playwrightInstance' #221

Closed MindaugasButkus closed 9 months ago

MindaugasButkus commented 9 months ago

I am getting this error when running integration tests with mocha@10.2.0) and @itwin/oidc-signin-tool@4.3.0: Error: global leak(s) detected: '_playwrightInstance' at Runner.emit (node:events:529:35) at processTicksAndRejections (node:internal/process/task_queues:95:5)

The issue appeared once I upgraded to version 4.3.0 of oidc-signin-tool from version 4.1.2. The issue disappears when downgraded back to 4.1.2.

Node.js version: 18.18.2 OS: Ubuntu 22.04.3

MichaelBelousov commented 9 months ago

This is probably because in 4.3.0 we started dynamically importing playwright, so mocha's leak detection probably no longer detects it at startup... You should probably just ignore this leak in your mocha configuration... Thoughts?

MindaugasButkus commented 9 months ago

It would be best if no custom configuration would be required for this. Or at least it should be well documented. What about other test runners? Would they also complain about this?

MichaelBelousov commented 9 months ago

I am not sure how many people use tests that fail on global leak detection. Afaict you must be using mocha's --check-leaks, perhaps it is the default but that isn't implied by their docs

We will definitely document this, I don't think we have a clean way around this.

In order to support users bring their own playwright instance, we need to conditionally late import playwright whenever the user needs it. Playwright will refuse to import if we try to import our own playwright when the user has already imported their own. But that late import isn't caught by mocha's global leak detection, hence the new error.

In the future we can do a breaking change and make playwright an (optional?) peer dependency of this package so users that bring their own won't cause a double import. For now I think we'll just document how to configure mocha to ignore this, in the README.

An alternative would be we supply a cleanup method which removes that global, but since that is internal to playwright, we would probably need to make sure they have their own deinit routine that we can transitively expose. I don't think this is a large issue so I think leak configuration override is fine.

cc @benpolinsky @aruniverse

ben-polinsky commented 9 months ago

Another option: If we separate the login actions/logic into a different package then this wouldn't be an issue.