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

fix: sandbox without loosing document.location context #1

Open
wants to merge 1 commit into
base: beta
Choose a base branch
from

Conversation

goooseman
Copy link

What's changing, and why

A bug was found if tested code contains ServiceWorker

Steps to reproduce

  1. Have a test which contains ServiceWorker
    1. ServiceWorker may be either in the tested or test code. For example, I was using MSW mocking library.
  2. Run tests

Expected

ServiceWorker to register.

Actual

  • "InvalidStateError: Failed to get ServiceWorkerRegistration objects: The document is in an invalid state" in Chrome
  • Script origin does not match the registering client's origin in Safari

Technical details

This problem happens, because TestSandbox abstraction executes each individual test in a new iframe to isolate those. This iframe is created without the src property, so all the tests are executed in the empty page context: if document.location is debugged, it appears as about:blank without any hostname.

This is a problem to register a ServiceWorker, and also it may bring incompatibility issues to other browser APIs.

A proposed way to fix it:

  • Distribute an additional empty iframe-entrypoint.html file
  • Make iframe to be rendered inside this empty html file being served by the HTTPs server already used in the project

This way the test is executed inside a secure HTTPs context. ServiceWorkers are tested to behave properly, MSW mocks fine.

What else might be impacted?

This is a relatively small change in terms of production code, but it also brings a change to the testing infrastructure of the project.

Previously only unit tests has been present in the project.

This PR adds a new integration tests folder, just because I believe the best way to implement such a test is by integration test. Such a test may bring additional confidence this functionality is not broken in the future releases especially if internal implementation (e.g. changing iframe isolation to any other) is changed.

This integration test uses esm-unit to run esm-unit tests. And nevertheless I personally love the romance of such a quine, I also admit this idea may be different from the project's ideology and roadmap. And also I confirm that the tests run-time has been dramatically increased.

So please provide me with an input about this change, I see 3 different possibilities:

  1. integration tests are kept together with unit ones, everything stays like in this PR
  2. unit ones are rewritten to the new integration structure, in this case npm test can be rewritten to run bin/esm-unit to better match real-world usage and the tool itself may be used to collect coverage itself. I am enthusiastic to do such a migration.
  3. integration tests are removed and this test is rewritten in unit-test fashion

Checklist

[x] Tested with --debug
[x] Tested with headless command line
[x] Documentation (function/class docs, comments, etc.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant