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

Code injection config #5133

Closed
brettz9 opened this issue Mar 14, 2017 · 8 comments
Closed

Code injection config #5133

brettz9 opened this issue Mar 14, 2017 · 8 comments

Comments

@brettz9
Copy link
Contributor

brettz9 commented Mar 14, 2017

In working on our IndexedDB polyfill, leveraging your test server when doing browser testing appears most promising, but we need to inject our polyfill code into the IndexedDB tests, and it is not convenient for us to keep monkey-patching upon every test update.

Would you be open to a PR to allow passing in a command-line argument (or adding a setting on config.json) to be able to designate say a callback or file with a callback that could selectively inject code into the desired test files before they are served?

While I suspect this might be seen as beyond your immediate scope, depending on how you might wish it implemented, it should only involve a few lines of code. (Our specific need can be met by overriding the wptserve/handlers FileHandler behavior as used by web-platform-tests (via wpt-tools/serve) for HTML files served by IndexedDB.)

@annevk
Copy link
Member

annevk commented Mar 14, 2017

@inexorabletash you should probably comment on this.

@brettz9 one thing you might be able to is to refactor the existing tests using the .any.js scheme: http://web-platform-tests.org/writing-tests/testharness.html. Then you can just write your own boilerplate wrapper for the JavaScript.

@inexorabletash
Copy link
Member

Sorry for delay in responding.

I don't think there's an IndexedDB-specific request here - you'd want this for any tests exercising a polyfill.

As @annevk mentions the place to do this would be something closer to wptserve (Blink doesn't run .any.js tests yet so I can't wholeheartedly endorse that particular approach yet)

Have you considered testharnessreport.js as a temporary (hacky) hook? That's pulled in for all window tests at least, and that file is intended to be overridden by test servers to map the output into whatever the CI system is expecting. Not the right design, but may unblock you.

@brettz9
Copy link
Contributor Author

brettz9 commented Apr 11, 2017

Thanks, @annevk , but I'd like to avoid making individual wrappers for all of the tests and keep them reflecting the same names, as I would imagine would be necessary.

And thanks also, @inexorabletash . As far as wptserve, I'd be happy to supply a PR to it assuming you'd be open to a PR here too as I think it would make sense (if not be necessary) to pass the config through web-platform-tests.

As far as testharnessreport.js, the problems are that:

  1. If I use testharnessreport.js directly, the only way I get it working is to do a synchronous XHR and eval, but this suffers from losing source map info, at least where I'm currently debugging (in Safari) which is a real problem for debugging.
  2. If I use testharness_runner.html (which is nicely in .gitignore already unlike testharnessreport.js), I can overcome the source map limitation (with a few modifications I imagine you might be open to accepting) but the solution doesn't allow individual files to be run outside of the runner. Basically, I add tools/runner/index.html as an iframe to my testharness_runner.html, but the current config doesn't allow passing in settings from outside of the file so if I also modify TestControl.prototype.get_testharness_settings in runner.js to assign based on any pre-existing window.testharness_properties (i.e., those which I inject from my testharness_runner.html, I can ensure that it receives a callback sent by my testharness_runner.html. I then need to modify testharnessreport.js to look for this callback property and if present, execute it (passing in its own window object) and this gets things to work, but again, it requires execution from the test runner.

@annevk
Copy link
Member

annevk commented Apr 11, 2017

@brettz9 the wrapper can be auto-generated from a Python script or some such though. No need to tailor it for each instance.

@brettz9
Copy link
Contributor Author

brettz9 commented Apr 11, 2017

🤦 Thanks!

@brettz9
Copy link
Contributor Author

brettz9 commented Apr 12, 2017

I created a Node script to populate the .any.js files with a wrapper, and it is now working (courtesy of an iframe pointing to our polyfill script and another script that copies the polyfill into its own dynamically-created child iframe pointing to the W3C test).

I am therefore now able to get individual files loading with our polyfill (and without using eval and breaking source maps), albeit with our testers needing to know of the need to add ".any.js" to any file they wish to test.

However, when I use the test runner, the manifest file it builds does not seem to base its listing from the directories (e.g., IndexedDB where I've also added our .any.js files), so these .any.html files can't apparently be referenced within the runner. Does anyone know off the top of their head where this is set and whether it can be configured dynamically without disturbing the repo?

If it is not feasible to get this working, as previously mentioned, I am able to get testharnessreport.js working, so I could submit a PR to better support customization to avoid future conflicts if testharnessreport.js may be further modified on your end.

brettz9 added a commit to brettz9/IndexedDBShim that referenced this issue Apr 12, 2017
…t one global `setGlobalVars`

- Testing (W3C): Utilize `.any.js` scheme to unobtrusively allow testing of polyfill by web-platform-tests (as per <web-platform-tests/wpt#5133 (comment)>)
- Docs (README/TESTING): Move from README to `docs` directory and indicate lack of currency of Cordova/PhoneGap info
- Docs (CONTRIBUTING): Move to `docs` directory
- Testing (Grunt): Add CORS headers to allow `web-platform.test` domain to access our polyfill (for W3C tests)
- Testing (Grunt): Add `connect-watch` task to avoid having to rebuild
@annevk
Copy link
Member

annevk commented Apr 12, 2017

I think you shouldn't use the test runner. It's unstable. @jgraham can correct me if I'm wrong, but I'm pretty sure I'm not.

@brettz9
Copy link
Contributor Author

brettz9 commented Apr 16, 2017

I've added two minor tweaks at w3c/testharness.js#263 and w3c/wpt-tools#203 which allow unobtrusive mods to get the runner working for our polyfill injecter (though the PRs could be used by any polyfill tester). Since I don't need any changes within this repo, I'll close now. Thanks for your help!

@brettz9 brettz9 closed this as completed Apr 16, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants