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

[WIP] fix(testing): don't allow test files to call Deno.exit() #12938

Closed
wants to merge 10 commits into from

Conversation

bartlomieju
Copy link
Member

Just to get the prototype going, current solution is wrong as it would
error out for all tests in denoland/std#1628

Closes #12888

@bartlomieju
Copy link
Member Author

A bunch of cases to consider:

  • test file calls Deno.exit() in top level - this should be an immediate failure
  • test file calls Deno.exit() in "unload" event handler - this one is tricky because any non-zero code should be immediate failure, but 0 exit code should still pass unless there were test failures
  • test file sets exit code using op_set_exit_code - this one is also tricky, I think it should behave like previous example

@bnoordhuis
Copy link
Contributor

W.r.t. (2) and (3): how do you stop the script under test in the Deno.exit(0) case? isolate.terminate_execution()?

@bartlomieju
Copy link
Member Author

W.r.t. (2) and (3): how do you stop the script under test in the Deno.exit(0) case? isolate.terminate_execution()?

Currently I don't but I'm open to suggestions.

@bnoordhuis
Copy link
Contributor

Discussed briefly OOB: Deno.exit() should not return so isolate.terminate_execution() may be the way to go (or suspend the worker thread until program exit but that's very meh.)

@bartlomieju
Copy link
Member Author

Another crux I found that needs to be figured out: the test runner can execute multiple test files in parallel (with --jobs flag). This is done on separate threads with the main thread being a "coordinator". This is problematic because "exit code" is effectively a global (stored in deno_runtime) so threads can race between each other and override exit code value.

@bartlomieju
Copy link
Member Author

@lucacasonato suggested to use Arc<AtomicUsize> that will be shared between "main worker" and its descendent "web workers"; that way we can always grab it's value from "main" worker when process shut be shutting down. Sounds like a reasonable solution, I will look into implementing that.

@bartlomieju
Copy link
Member Author

Blocked by #13034

@bartlomieju
Copy link
Member Author

This is now unblocked, I'll try to wrap it up this weekend.

@bartlomieju
Copy link
Member Author

bartlomieju commented Dec 17, 2021

Discussed with @lucacasonato offline; we believe that any call to Deno.exit() in top level code in test files should result in immediate termination of isolate without heuristics for exit code; resulting in failing tests. If your code calls Deno.exit() it's most likely an application (in contrast to library) and thus you should be integration testing it using Deno.run().

AaronO added a commit to AaronO/deno that referenced this pull request Dec 29, 2021
Enabling op-middleware for overrides in lieu of imperative .replace_op() etc...

Impacts denoland#13219,  denoland#12938, denoland#13122
AaronO added a commit that referenced this pull request Dec 29, 2021
Enabling op-middleware for overrides in lieu of imperative .replace_op() etc...

Impacts #13219,  #12938, #13122
bartlomieju pushed a commit that referenced this pull request Jan 5, 2022
Enabling op-middleware for overrides in lieu of imperative .replace_op() etc...

Impacts #13219,  #12938, #13122
@stale
Copy link

stale bot commented Mar 6, 2022

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Mar 6, 2022
@stale stale bot closed this Mar 13, 2022
@bartlomieju bartlomieju reopened this Mar 13, 2022
@stale stale bot removed the stale label Mar 13, 2022
@dsherret dsherret marked this pull request as draft March 18, 2022 22:34
@stale
Copy link

stale bot commented May 25, 2022

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label May 25, 2022
@stale stale bot closed this Jun 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Calling Deno.exit inside top level deno test isolates exits the process
2 participants