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

Add dummy of test_driver #14

Merged
merged 4 commits into from
Mar 18, 2019
Merged

Conversation

sttk
Copy link
Contributor

@sttk sttk commented Mar 8, 2019

There are some tests in wpt, which use test_driver and cause error on wpt-runner.

This pr sets dummy code of test_driver to response when a test html uses /resources/testdriver.js.

domenic
domenic previously requested changes Mar 9, 2019
Copy link
Owner

@domenic domenic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very cool, thank you! Some work to do still.

.gitignore Outdated
@@ -1,3 +1,17 @@
/node_modules/
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please revert these changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've reverted .gitignore, but /test/tests/resources is added.

if (!window.document.contains(element)) {
return Promise.reject(new Error("element in different document or shadow tree"));
}
return new Promise(function(resolve) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return Promise.resolve(). Or, maybe actually implement it?

Same for all other very-long promise creations in this code.


function getDummyCodeOfTestDriver() {
return `
(function() {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This IIFE isn't needed.

return `
(function() {
window.test_driver = {
bless: function(intent, action) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use bless(intent, action) {.

"copy-testharness": "copyfiles -u 2 wpt/resources/testharness.js testharness/"
"prepare": "npm run copy-testharness && npm run copy-resources",
"copy-testharness": "copyfiles -u 2 wpt/resources/testharness.js testharness/",
"copy-resources": "copyfiles -u 2 wpt/resources/testharness.js test/tests/resources/"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't needed for preparing the published package, so it seems like this should be a pretest hook, not part of prepare.

test/runner.js Outdated
const wptRunner = require("..");

const testcases = require("./testcases.json");
const filter = testPath => (testcases[testPath] === true);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather just enumerate all the files in the test/tests directory using the fs APIs; then we don't need to manually update a testcases.json file.


promise_test(async t => {
await test_driver.send_keys(window.document.body);
assert_true(true);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These assert_trues do nothing, so they're best removed.

}, "test");

promise_test(t => {
return Promise.resolve(() => {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Passing a function to Promise.resolve() just resolves the promise with that function. Probably you should just not pass anything.

@@ -208,3 +218,43 @@ function recursiveReaddir(dirPath) {
});
});
}

function getDummyCodeOfTestDriver() {
return `
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps it'd be better to move this to its own file, and fs.readFileSync it at the top of wpt-runner.js.

@sttk
Copy link
Contributor Author

sttk commented Mar 10, 2019

@domenic Thanks for your reviewing. I've modified what you pointed out.

@domenic domenic dismissed their stale review March 13, 2019 21:43

Fixed

@domenic
Copy link
Owner

domenic commented Mar 13, 2019

@sttk I made some further simplifications and fixes; please take a look and let me know what you think!

@sttk
Copy link
Contributor Author

sttk commented Mar 14, 2019

@domenic Oh, I misread your comment as not to create testcases.json manually.
You said testcases.json itself is unnecessary.

I think it's also good, and I'm sorry that I forgot to lint.
Thanks for good modifications!

@domenic domenic merged commit 6004b93 into domenic:master Mar 18, 2019
@sttk
Copy link
Contributor Author

sttk commented Mar 19, 2019

@domenic Thank you for merging!

@sttk sttk deleted the add_dummy_testdriver branch March 19, 2019 13:06
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.

2 participants