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

Temporary files not beeing removed after tests are completed (Jest, Windows) #229

Closed
8 tasks done
armano2 opened this issue Jan 28, 2020 · 18 comments
Closed
8 tasks done
Labels
bug wontfix workaround a workaround is available

Comments

@armano2
Copy link

armano2 commented Jan 28, 2020

Operating System

  • Linux
  • Windows 7
  • Windows 10
  • MacOS
  • other:

NodeJS Version

  • 8.x
  • 10.x
  • 12.x
  • 13.x
  • other:

Tmp Version

0.1.0 and master

Expected Behavior

Temporary files should be removed after test is finishes

Experienced Behavior

Temporary files are not removed


I'm experiencing this issues within jest tests, this can be reproduced in

If needed i can prepare small repo project or create test scenario

@armano2 armano2 changed the title Temporary files not beeing removed after tests are completed Temporary files not beeing removed after tests are completed (Jest, Windows) Jan 28, 2020
@armano2
Copy link
Author

armano2 commented Jan 28, 2020

i did some digging and this seems to be issue with jest isolating tests
process events are not being triggered and cleanup is not triggered

@silkentrance
Copy link
Collaborator

I will have a look at this.

@silkentrance
Copy link
Collaborator

@armano2 Ah I see. Now, there was a change to the rmdirSync API which expects a second optional parameter. ATM we will pass the next handler to this function, too, which causes node fs rmdirSync to fail.

There is already a bug fix for that. Perhaps you can try out node-tmp/master?

I am currently working on stabilising the solution and getting the event handling stuff right. Hopefully @raszi will then make a new release 😄

Other than that: this should be a duplicate of #209

@silkentrance
Copy link
Collaborator

Reopening, seeing that you already tried out master.

@silkentrance silkentrance reopened this Jan 29, 2020
@silkentrance
Copy link
Collaborator

I just tried this with the latest master and running the tests from your project.

Turns out that the temporary objects are cleaned up as expected.

However, the SIGINT handler will not be called on CTRL-C. But this is a different issue.

Can you please check this again with the latest master?

@silkentrance
Copy link
Collaborator

silkentrance commented Jan 29, 2020

There is a bug in current master on dirSync/removeCallback. (next is not a function.)
The problem lies in the rimrafSync wrapper for removing directories. Will open a new issue for this, see #230.

@silkentrance
Copy link
Collaborator

@armano2 is this the issue that the SIGINT handler will not be called?

@armano2
Copy link
Author

armano2 commented Jan 29, 2020

yes, i did some debugging and it looks like process.on and process.addEventListener is not triggered at all

@silkentrance
Copy link
Collaborator

silkentrance commented Jan 29, 2020

@armano2 thank you.

I just did some debugging and by looking at the jest code, it seems that they actually switched over to worker threads. In the past, they would just import the test suite module and provide it with a more or less empty sandbox context. This lead to the fact that tmp got loaded multiple times, installing its listeners over and over again.

With the workers now in place, the test suite module will be loaded in the context of the so spawned worker thread and tmp will be loaded once per worker thread, no longer installing its listeners multiple times.

In order to test this, I added the import for tmp to jest-cli and put some debug output in the safe listener installer, logging the available listeners. It shows that the process object is different from the worker thread's process.

https://nodejs.org/api/worker_threads.html#worker_threads_class_worker

And ATM I do not see any way of making SIGINT/EXIT handlers work in that scenario as these signals will not be passed to the worker thread.

@silkentrance
Copy link
Collaborator

Here is a small diagram on how the process environment looks like and which handlers will be called

main process (standard SIGINT handler is used)
-- worker thread (will be terminated as soon as the main process is interrupted, tmp exit handler will not be called)

@silkentrance
Copy link
Collaborator

silkentrance commented Jan 29, 2020

The only work around for this would be to use inband testing alone using the -i option.

However, initial tests have shown that this will also not work.

@silkentrance
Copy link
Collaborator

I dug somewhat deeper and the enableWorkerThreads option is not set, so jest is actually using child processes.

@silkentrance
Copy link
Collaborator

Nah, it is basically the same as with the worker threads. SIGINT will be received by the main process and the child processes will just be terminated.
However, the exit handler should still be called during normal execution, similarly to our own tests.

@silkentrance
Copy link
Collaborator

@armano2 I have found a solution to this, see the gist https://gist.github.com/silkentrance/f0e1ff1d689891e124e2a8b73bd31299

@silkentrance
Copy link
Collaborator

Somehow all this runincontext stuff will break the process event listeners.

@silkentrance silkentrance added workaround a workaround is available wontfix and removed investigation labels Jan 30, 2020
@armano2
Copy link
Author

armano2 commented Jan 30, 2020

thank you for investigating it, maybe you should consider, creating guide for other people in readme how to use within jest :>

@silkentrance
Copy link
Collaborator

@armano2 I an currently working on a package for doing exactly that. I will reference the package from the documentation. Got a little bit carried away, though :)

@silkentrance
Copy link
Collaborator

@armano2 can we close this for now?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug wontfix workaround a workaround is available
Projects
None yet
Development

No branches or pull requests

2 participants