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

test: improve WPT runner to summarize status and update WPT #33297

Closed
wants to merge 2 commits into from

Conversation

joyeecheung
Copy link
Member

@joyeecheung joyeecheung commented May 8, 2020

test: update WPT interfaces and hr-time

This commit updates the interfaces to
https://github.com/web-platform-tests/wpt/tree/8ada332aea/interfaces
and updates the hr-time test status:

  • window-worker-timeOrigin.window.js should be skipped because we
    don't implement Blob
  • idlharness.any.js should be skipped since the IDL parser needs
    to be updated, but the parser update would also result in
    an update of the test harness which in turn requires updates of
    other tests. We need to fix the URL implementation first,
    and then update the harness and all the tests.

test: refactor WPTRunner

  • Print test results as soon as they are available, instead of
    until after all the tests are complete. This helps us printing
    tests whose completion callback is not called because of
    failures.
  • Run the scripts specified by // META: script= one by one
    instead of concatenating them first for better error stack
    traces.
  • Print a status summary when the test process is about to exit.
    This can be used as reference for updating the status file.

For example the stderr output of
out/Release/node test/wpt/test-console.js would be:

{
  'idlharness.any.js': {
    fail: {
      expected: [
        'assert_equals: operation has wrong .length expected 1 but got 0'
      ]
    }
  }
}
Ran 4/4 tests, 0 skipped, 3 passed, 1 expected failures, 0 unexpected failures
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label May 8, 2020
@joyeecheung
Copy link
Member Author

These are the status updates in the second commit

diff --git a/test/wpt/status/encoding.json b/test/wpt/status/encoding.json
index b51dde2aae..e06eda3699 100644
--- a/test/wpt/status/encoding.json
+++ b/test/wpt/status/encoding.json
@@ -2,11 +2,23 @@
   "api-basics.any.js": {
     "requires": ["small-icu"]
   },
+  "api-invalid-label.any.js": {
+    "skip": "location is not defined"
+  },
+  "api-replacement-encodings.any.js": {
+    "requires": ["small-icu"],
+    "fail": "unsupported encodings csiso2022kr, hz-gb-2312, iso-2022-cn, iso-2022-cn-ext, iso-2022-kr, replacement"
+  },
+  "encodeInto.any.js": {
+    "fail": "encodeInto() doesn't support types other than Uint8Array"
+  },
   "textdecoder-fatal-streaming.any.js": {
-    "requires": ["small-icu"]
+    "requires": ["small-icu"],
+    "fail": "WPT harness expects vanilla Errors"
   },
   "textdecoder-fatal.any.js": {
-    "requires": ["small-icu"]
+    "requires": ["small-icu"],
+    "fail": "WPT harness expects vanilla Errors"
   },
   "textdecoder-ignorebom.any.js": {
     "requires": ["small-icu"]
@@ -15,7 +27,8 @@
     "requires": ["small-icu"]
   },
   "textdecoder-utf16-surrogates.any.js": {
-    "requires": ["small-icu"]
+    "requires": ["small-icu"],
+    "fail": "WPT harness expects vanilla Errors"
   },
   "iso-2022-jp-decoder.any.js": {
     "requires": ["full-icu"],
diff --git a/test/wpt/status/html/webappapis/microtask-queuing.json b/test/wpt/status/html/webappapis/microtask-queuing.json
index dc13452b99..7e3d11494d 100644
--- a/test/wpt/status/html/webappapis/microtask-queuing.json
+++ b/test/wpt/status/html/webappapis/microtask-queuing.json
@@ -2,6 +2,9 @@
   "queue-microtask-exceptions.any.js": {
     "fail": "Node.js does not have a global addEventListener function"
   },
+  "queue-microtask.any.js": {
+    "fail": "WPT harness expects vanilla Errors"
+  },
   "queue-microtask.window.js": {
     "fail": "MutationObserver is not implemented"
   }
diff --git a/test/wpt/status/url.json b/test/wpt/status/url.json
index afd5acdcbf..c86dcde64c 100644
--- a/test/wpt/status/url.json
+++ b/test/wpt/status/url.json
@@ -4,11 +4,18 @@
     "skip": "TODO: port from .window.js"
   },
   "historical.any.js": {
-    "requires": ["small-icu"]
+    "requires": ["small-icu"],
+    "fail": "WPT harness expects vanilla Errors"
   },
   "urlencoded-parser.any.js": {
     "fail": "missing Request and Response"
   },
+  "url-setters-stripping.any.js": {
+    "fail": "Implement https://github.com/whatwg/url/issues/419/"
+  },
+  "urlsearchparams-constructor.any.js": {
+    "fail": "WPT harness expects vanilla Errors"
+  },
   "idlharness.any.js": {
     "fail": "getter/setter names are wrong, etc."
   }

The upstream did some refactoring to assert_throws and now it expects e.constructor === constructor which results in several failures. I wonder if we could change it to instanceof in the upstream (however we would still need to override the globals in the testing context)

@nodejs-github-bot
Copy link
Collaborator

@joyeecheung
Copy link
Member Author

hmm, looks like our whatwg URL tests using urltestdata.json all failed after the update. I'll try to pin point the change in the spec the breakage come from.

Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

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

Looks good, thank you for the good and clear commit messages they made this easier to review.

@joyeecheung
Copy link
Member Author

Since the second commit is blocked by #33315, I will remove the URL test updates

@joyeecheung
Copy link
Member Author

joyeecheung commented May 11, 2020

hm, actually, I'll just drop the second commit - the URL tests now depend on the new assert_throw utilities in the harness which were refactored later than the breaking changes, so there is no way to update the harness (and other subsets) without updating the URL tests (other than floating patches, which would just add more churn compared to waiting for them to be fixed before another test update)

@nodejs-github-bot
Copy link
Collaborator

This commit updates the interfaces to
https://github.com/web-platform-tests/wpt/tree/8ada332aea/interfaces
and updates the hr-time test status:

- `window-worker-timeOrigin.window.js` should be skipped because we
  don't implement `Blob`
- `idlharness.any.js` should be skipped since the IDL parser needs
  to be updated, but the parser update would also result in
  an update of the test harness which in turn requires updates of
  other tests. We need to fix the URL implementation first,
  and then update the harness and all the tests.
- Print test results as soon as they are available, instead of
  until after all the tests are complete. This helps us printing
  tests whose completion callback is not called because of
  failures.
- Run the scripts specified by `// META: script=` one by one
  instead of concatenating them first for better error stack
  traces.
- Print a status summary when the test process is about to exit.
  This can be used as reference for updating the status file.

For example the stderr output of
`out/Release/node test/wpt/test-console.js` would be:

```
{
  'idlharness.any.js': {
    fail: {
      expected: [
        'assert_equals: operation has wrong .length expected 1 but got 0'
      ]
    }
  }
}
Ran 4/4 tests, 0 skipped, 3 passed, 1 expected failures, 0 unexpected failures
```
@nodejs-github-bot
Copy link
Collaborator

@joyeecheung
Copy link
Member Author

This refactoring uncovered issues with #32790 so I added an additional commit to update the hr-time status as well as updating all the WebIDL definitions. See the commit log for more explanation. I also edited the second commit to run the scripts specified by // META: script= one by one instead of concatenating them first for better error stack traces.

@addaleax @benjamingr PTAL again, thanks!

also cc @targos

@joyeecheung
Copy link
Member Author

BTW the first commit uses nodejs/node-core-utils#418 to pull dom.idl and html.idl into the repo

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

Still LGTM 👍

joyeecheung added a commit that referenced this pull request May 13, 2020
This commit updates the interfaces to
https://github.com/web-platform-tests/wpt/tree/8ada332aea/interfaces
and updates the hr-time test status:

- `window-worker-timeOrigin.window.js` should be skipped because we
  don't implement `Blob`
- `idlharness.any.js` should be skipped since the IDL parser needs
  to be updated, but the parser update would also result in
  an update of the test harness which in turn requires updates of
  other tests. We need to fix the URL implementation first,
  and then update the harness and all the tests.

PR-URL: #33297
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
joyeecheung added a commit that referenced this pull request May 13, 2020
- Print test results as soon as they are available, instead of
  until after all the tests are complete. This helps us printing
  tests whose completion callback is not called because of
  failures.
- Run the scripts specified by `// META: script=` one by one
  instead of concatenating them first for better error stack
  traces.
- Print a status summary when the test process is about to exit.
  This can be used as reference for updating the status file.

For example the stderr output of
`out/Release/node test/wpt/test-console.js` would be:

```
{
  'idlharness.any.js': {
    fail: {
      expected: [
        'assert_equals: operation has wrong .length expected 1 but got 0'
      ]
    }
  }
}
Ran 4/4 tests, 0 skipped, 3 passed, 1 expected failures, 0 unexpected failures
```

PR-URL: #33297
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
@joyeecheung
Copy link
Member Author

Landed in e1b90be...365ddb3, thanks

codebytere pushed a commit that referenced this pull request May 16, 2020
This commit updates the interfaces to
https://github.com/web-platform-tests/wpt/tree/8ada332aea/interfaces
and updates the hr-time test status:

- `window-worker-timeOrigin.window.js` should be skipped because we
  don't implement `Blob`
- `idlharness.any.js` should be skipped since the IDL parser needs
  to be updated, but the parser update would also result in
  an update of the test harness which in turn requires updates of
  other tests. We need to fix the URL implementation first,
  and then update the harness and all the tests.

PR-URL: #33297
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
codebytere pushed a commit that referenced this pull request May 16, 2020
- Print test results as soon as they are available, instead of
  until after all the tests are complete. This helps us printing
  tests whose completion callback is not called because of
  failures.
- Run the scripts specified by `// META: script=` one by one
  instead of concatenating them first for better error stack
  traces.
- Print a status summary when the test process is about to exit.
  This can be used as reference for updating the status file.

For example the stderr output of
`out/Release/node test/wpt/test-console.js` would be:

```
{
  'idlharness.any.js': {
    fail: {
      expected: [
        'assert_equals: operation has wrong .length expected 1 but got 0'
      ]
    }
  }
}
Ran 4/4 tests, 0 skipped, 3 passed, 1 expected failures, 0 unexpected failures
```

PR-URL: #33297
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
@codebytere codebytere mentioned this pull request May 18, 2020
codebytere pushed a commit that referenced this pull request Jun 7, 2020
This commit updates the interfaces to
https://github.com/web-platform-tests/wpt/tree/8ada332aea/interfaces
and updates the hr-time test status:

- `window-worker-timeOrigin.window.js` should be skipped because we
  don't implement `Blob`
- `idlharness.any.js` should be skipped since the IDL parser needs
  to be updated, but the parser update would also result in
  an update of the test harness which in turn requires updates of
  other tests. We need to fix the URL implementation first,
  and then update the harness and all the tests.

PR-URL: #33297
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
codebytere pushed a commit that referenced this pull request Jun 7, 2020
- Print test results as soon as they are available, instead of
  until after all the tests are complete. This helps us printing
  tests whose completion callback is not called because of
  failures.
- Run the scripts specified by `// META: script=` one by one
  instead of concatenating them first for better error stack
  traces.
- Print a status summary when the test process is about to exit.
  This can be used as reference for updating the status file.

For example the stderr output of
`out/Release/node test/wpt/test-console.js` would be:

```
{
  'idlharness.any.js': {
    fail: {
      expected: [
        'assert_equals: operation has wrong .length expected 1 but got 0'
      ]
    }
  }
}
Ran 4/4 tests, 0 skipped, 3 passed, 1 expected failures, 0 unexpected failures
```

PR-URL: #33297
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
@codebytere codebytere mentioned this pull request Jun 9, 2020
@panva panva mentioned this pull request Jun 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants