Exceptions thrown in a worker's setup function don't fail the test until it times out
#29777
Labels
Comments
This was referenced Jul 25, 2021
|
As far as I can tell, this is caused because It seems like one of the cases where the There are probably other cases where that condition is needed. |
|
The same thing happens if you reject a remote |
This was referenced Aug 10, 2021
|
I'll be documenting here the breakage I find from changing this in #29970:
Edit: Given the many tests that break, it's probably best to go back to the drawing board and try something else. Edit 2: I've since repurposed PR #29970 to have a different fix to this problem which doesn't seem to break any tests. |
andreubotella
pushed a commit
that referenced
this issue
Aug 11, 2021
…ontexts `Tests.prototype.all_done` used to treat the presence of at least one test as a necessary condition for all tests to be done. This would be false in cases where an exception is thrown inside the `setup` function, but the code path that runs in that case doesn't use the `all_done()` function. However, if an exception is thrown inside the `setup` function in a worker test, `tests.all_done()` is called in that code path, which results in the completion message from the worker being ignored. This change fixes this by changing the condition to require the presence of either tests or remote contexts. For service worker tests, however, that is not enough, since service worker registration is asynchronous. This change lets the `fetch_tests_from_worker` function take a promise or an async function that returns the worker, and it guarantees that the test won't be considered complete until the promise is resolved. The HTML boilerplate for service worker tests is also changed in turn. Closes #29777.
andreubotella
pushed a commit
that referenced
this issue
Aug 11, 2021
…remotes `Tests.prototype.all_done` used to treat the presence of at least one test as a necessary condition for all tests to be done. This would be false in cases where an exception is thrown inside the `setup` function, but the code path that runs in that case doesn't use the `all_done()` function. However, if an exception is thrown inside the `setup` function in a worker test, `tests.all_done()` is called in that code path, which results in the completion message from the worker being ignored. This change fixes this by changing the condition to require the presence of either tests or remote contexts. For service worker tests, however, that is not enough, since service worker registration is asynchronous. This change lets the `fetch_tests_from_worker` function take a promise or an async function that returns the worker, and it guarantees that the test won't be considered complete until the promise is resolved. The HTML boilerplate for service worker tests is also changed in turn. Closes #29777.
andreubotella
pushed a commit
that referenced
this issue
Aug 31, 2021
…remotes `Tests.prototype.all_done` used to treat the presence of at least one test as a necessary condition for all tests to be done. This would be false in cases where an exception is thrown inside the `setup` function, but the code path that runs in that case doesn't use the `all_done()` function. However, if an exception is thrown inside the `setup` function in a worker test, `tests.all_done()` is called in that code path, which results in the completion message from the worker being ignored. This change fixes this by changing the condition to require the presence of either tests or remote contexts. For service worker tests, however, that is not enough, since service worker registration is asynchronous. This change lets the `fetch_tests_from_worker` function take a promise or an async function that returns the worker, and it guarantees that the test won't be considered complete until the promise is resolved. The HTML boilerplate for service worker tests is also changed in turn. Closes #29777.
andreubotella
pushed a commit
that referenced
this issue
Sep 10, 2021
…remotes `Tests.prototype.all_done` used to treat the presence of at least one test as a necessary condition for all tests to be done. This would be false in cases where an exception is thrown inside the `setup` function, but the code path that runs in that case doesn't use the `all_done()` function. However, if an exception is thrown inside the `setup` function in a worker test, `tests.all_done()` is called in that code path, which results in the completion message from the worker being ignored. This change fixes this by changing the condition to require the presence of either tests or remote contexts. Closes #29777.
jgraham
pushed a commit
that referenced
this issue
Sep 13, 2021
…remotes `Tests.prototype.all_done` used to treat the presence of at least one test as a necessary condition for all tests to be done. This would be false in cases where an exception is thrown inside the `setup` function, but the code path that runs in that case doesn't use the `all_done()` function. However, if an exception is thrown inside the `setup` function in a worker test, `tests.all_done()` is called in that code path, which results in the completion message from the worker being ignored. This change fixes this by changing the condition to require the presence of either tests or remote contexts. Closes #29777.
andreubotella
pushed a commit
to andreubotella/deno
that referenced
this issue
Sep 25, 2021
Classic workers were implemented in denoland#11338, which also enabled the WPT tests in the `workers` directory. However, the rest of WPT worker tests were not enabled because a number of them were hanging due to web-platform-tests/wpt#29777. Now that that WPT issue is fixed, the bulk of worker tests can be enabled. There are still a few tests that hang, and so haven't been enabled. In particular: - The following tests seem to hang because a promise fails to resolve. We can detect such cases in non-worker tests because the process will exit without calling the WPT completion callback, but in worker tests the worker message ops will keep the event loop running. This will be fixed when we add timeouts to WPT tests (denoland#9460). - `/fetch/api/basic/error-after-response.any.worker.html` - `/html/webappapis/microtask-queuing/queue-microtask-exceptions.any.worker.html` - `/webmessaging/message-channels/worker-post-after-close.any.worker.html` - `/webmessaging/message-channels/worker.any.worker.html` - `/websockets/Create-on-worker-shutdown.any.worker.html` - The following tests apparently hang because a promise rejection is never handled, which will kill the process in the main thread but not in workers (denoland#12221). - `/streams/readable-streams/async-iterator.any.worker.html` - `/workers/interfaces/WorkerUtils/importScripts/report-error-setTimeout-cross-origin.sub.any.worker.html` - `/workers/interfaces/WorkerUtils/importScripts/report-error-setTimeout-redirect-to-cross-origin.sub.any.worker.html` - `/workers/interfaces/WorkerUtils/importScripts/report-error-setTimeout-same-origin.sub.any.worker.html`
andreubotella
pushed a commit
to andreubotella/deno
that referenced
this issue
Oct 7, 2021
Classic workers were implemented in denoland#11338, which also enabled the WPT tests in the `workers` directory. However, the rest of WPT worker tests were not enabled because a number of them were hanging due to web-platform-tests/wpt#29777. Now that that WPT issue is fixed, the bulk of worker tests can be enabled. There are still a few tests that hang, and so haven't been enabled. In particular: - The following tests seem to hang because a promise fails to resolve. We can detect such cases in non-worker tests because the process will exit without calling the WPT completion callback, but in worker tests the worker message ops will keep the event loop running. This will be fixed when we add timeouts to WPT tests (denoland#9460). - `/fetch/api/basic/error-after-response.any.worker.html` - `/html/webappapis/microtask-queuing/queue-microtask-exceptions.any.worker.html` - `/webmessaging/message-channels/worker-post-after-close.any.worker.html` - `/webmessaging/message-channels/worker.any.worker.html` - `/websockets/Create-on-worker-shutdown.any.worker.html` - The following tests apparently hang because a promise rejection is never handled, which will kill the process in the main thread but not in workers (denoland#12221). - `/streams/readable-streams/async-iterator.any.worker.html` - `/workers/interfaces/WorkerUtils/importScripts/report-error-setTimeout-cross-origin.sub.any.worker.html` - `/workers/interfaces/WorkerUtils/importScripts/report-error-setTimeout-redirect-to-cross-origin.sub.any.worker.html` - `/workers/interfaces/WorkerUtils/importScripts/report-error-setTimeout-same-origin.sub.any.worker.html`
bartlomieju
pushed a commit
to denoland/deno
that referenced
this issue
Oct 8, 2021
Classic workers were implemented in #11338, which also enabled the WPT tests in the `workers` directory. However, the rest of WPT worker tests were not enabled because a number of them were hanging due to web-platform-tests/wpt#29777. Now that that WPT issue is fixed, the bulk of worker tests can be enabled. There are still a few tests that hang, and so haven't been enabled. In particular: - The following tests seem to hang because a promise fails to resolve. We can detect such cases in non-worker tests because the process will exit without calling the WPT completion callback, but in worker tests the worker message ops will keep the event loop running. This will be fixed when we add timeouts to WPT tests (#9460). - `/fetch/api/basic/error-after-response.any.worker.html` - `/html/webappapis/microtask-queuing/queue-microtask-exceptions.any.worker.html` - `/webmessaging/message-channels/worker-post-after-close.any.worker.html` - `/webmessaging/message-channels/worker.any.worker.html` - `/websockets/Create-on-worker-shutdown.any.worker.html` - The following tests apparently hang because a promise rejection is never handled, which will kill the process in the main thread but not in workers (#12221). - `/streams/readable-streams/async-iterator.any.worker.html` - `/workers/interfaces/WorkerUtils/importScripts/report-error-setTimeout-cross-origin.sub.any.worker.html` - `/workers/interfaces/WorkerUtils/importScripts/report-error-setTimeout-redirect-to-cross-origin.sub.any.worker.html` - `/workers/interfaces/WorkerUtils/importScripts/report-error-setTimeout-same-origin.sub.any.worker.html`
bartlomieju
pushed a commit
to bartlomieju/deno
that referenced
this issue
Oct 10, 2021
Classic workers were implemented in denoland#11338, which also enabled the WPT tests in the `workers` directory. However, the rest of WPT worker tests were not enabled because a number of them were hanging due to web-platform-tests/wpt#29777. Now that that WPT issue is fixed, the bulk of worker tests can be enabled. There are still a few tests that hang, and so haven't been enabled. In particular: - The following tests seem to hang because a promise fails to resolve. We can detect such cases in non-worker tests because the process will exit without calling the WPT completion callback, but in worker tests the worker message ops will keep the event loop running. This will be fixed when we add timeouts to WPT tests (denoland#9460). - `/fetch/api/basic/error-after-response.any.worker.html` - `/html/webappapis/microtask-queuing/queue-microtask-exceptions.any.worker.html` - `/webmessaging/message-channels/worker-post-after-close.any.worker.html` - `/webmessaging/message-channels/worker.any.worker.html` - `/websockets/Create-on-worker-shutdown.any.worker.html` - The following tests apparently hang because a promise rejection is never handled, which will kill the process in the main thread but not in workers (denoland#12221). - `/streams/readable-streams/async-iterator.any.worker.html` - `/workers/interfaces/WorkerUtils/importScripts/report-error-setTimeout-cross-origin.sub.any.worker.html` - `/workers/interfaces/WorkerUtils/importScripts/report-error-setTimeout-redirect-to-cross-origin.sub.any.worker.html` - `/workers/interfaces/WorkerUtils/importScripts/report-error-setTimeout-same-origin.sub.any.worker.html`
Gabisampaio
pushed a commit
to Gabisampaio/wpt
that referenced
this issue
Nov 18, 2021
…remotes `Tests.prototype.all_done` used to treat the presence of at least one test as a necessary condition for all tests to be done. This would be false in cases where an exception is thrown inside the `setup` function, but the code path that runs in that case doesn't use the `all_done()` function. However, if an exception is thrown inside the `setup` function in a worker test, `tests.all_done()` is called in that code path, which results in the completion message from the worker being ignored. This change fixes this by changing the condition to require the presence of either tests or remote contexts. Closes web-platform-tests#29777.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
While working on implementing classic workers in Deno in order to run the worker WPT tests, I noticed that an exception thrown in a worker's
setupfunction (in this case, in FileAPI/FileReaderSync.worker.html, since Deno doesn't yet supportFileReaderSync), will not fail the test immediately. Instead, the test only shows up as failed with the proper error message after it would have timed out.In the case of Deno, which is recognized as a
ShellTestEnvironment, this is bad because WPT doesn't time out tests in shell environments, so the test will run forever, and it will never call the completion callback.The issue here is that, once the completion message from the worker reaches the window environment,
Tests.prototype.completeis only called ifTests.prototype.all_donereturns true, which doesn't seem to be the case for exceptions thrown during a worker's setup.The text was updated successfully, but these errors were encountered: