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 the WPT runner and rename test files #24826

Closed
wants to merge 3 commits into from

Conversation

joyeecheung
Copy link
Member

@joyeecheung joyeecheung commented Dec 4, 2018

In preparation of porting other parts of WPT into test/wpt (I am currently working on performance-timeline and encoding).

test: remove reference to whatwg in file names under test/wpt

WPT covers standards in both W3C and WHATWG, as such it would be
strange to make this disparity explicit in our file names
(e.g. when testing standards that are solely in W3C, like
performance-timeline). Remove the reference to WHATWG will
also make the file names shorter.

Refs: #24823

test: improve WPT runner name matching

This patch:

  • Support wildcards(*) in WPT runner name matching (needed by e.g.
    encoding where all the tests requires i18n support in the build)
  • Print failure reasons when encountering an expected failure
  • Fix a bug in copyGlobalsFromObject (previously it copies
    properties from global instead of the given obj)

Previously an expected failure is printed as

[EXPECTED_FAILURE] response.formData() with input: %61+%4d%4D=

Now it is printed as

[EXPECTED_FAILURE] response.formData() with input: %61+%4d%4D=
missing Request and Response
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

WPT covers standards in both W3C and WHATWG, as such it would be
strange to make this disparity explicit in our file names
(e.g. when testing standards that are solely in W3C, like
performance-timeline). Remove the reference to WHATWG will
also make the file names shorter.
This patch:

- Support wildcards(*) in WPT runner name matching (needed by e.g.
  encoding where all the tests requires i18n support in the build)
- Print failure reasons when encountering an expected failure
- Fix a bug in copyGlobalsFromObject (previously it copies
  properties from `global` instead of the given `obj`)

Previously an expected failure is printed as

```
[EXPECTED_FAILURE] response.formData() with input: %61+%4d%4D=
```

Now it is printed as

```
[EXPECTED_FAILURE] response.formData() with input: %61+%4d%4D=
missing Request and Response
```
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Dec 4, 2018
@joyeecheung
Copy link
Member Author

cc @nodejs/testing @nodejs/url

@joyeecheung
Copy link
Member Author

@Trott
Copy link
Member

Trott commented Dec 6, 2018

@Trott
Copy link
Member

Trott commented Dec 6, 2018

@Trott
Copy link
Member

Trott commented Dec 6, 2018

Failure looks related?

https://ci.nodejs.org/job/node-test-commit-linux-containered/9206/nodes=ubuntu1604_sharedlibs_withoutintl_x64/testReport/

/home/iojs/build/workspace/node-test-commit-linux-containered/test/common/wpt.js:451
      reason: reasons.join('; ')
                      ^

TypeError: reasons.join is not a function
    at WPTRunner.skip (/home/iojs/build/workspace/node-test-commit-linux-containered/test/common/wpt.js:451:23)
    at WPTRunner.buildQueue (/home/iojs/build/workspace/node-test-commit-linux-containered/test/common/wpt.js:503:14)
    at WPTRunner.runJsTests (/home/iojs/build/workspace/node-test-commit-linux-containered/test/common/wpt.js:265:20)
    at Object.<anonymous> (/home/iojs/build/workspace/node-test-commit-linux-containered/test/wpt/test-url.js:19:8)
    at Module._compile (internal/modules/cjs/loader.js:723:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:734:10)
    at Module.load (internal/modules/cjs/loader.js:620:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:560:12)
    at Function.Module._load (internal/modules/cjs/loader.js:552:3)
    at Function.Module.runMain (internal/modules/cjs/loader.js:776:12)

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

LGTM

@joyeecheung joyeecheung added the blocked PRs that are blocked by other issues or PRs. label Dec 14, 2018
@joyeecheung
Copy link
Member Author

(Probably there are some loopholes in the logic for a nointl build, I'll investigate)

@joyeecheung
Copy link
Member Author

joyeecheung commented Dec 14, 2018

This should fix the nointl failure. CI: https://ci.nodejs.org/job/node-test-pull-request/19539/ ✔️

@joyeecheung joyeecheung added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed blocked PRs that are blocked by other issues or PRs. labels Dec 14, 2018
@Trott
Copy link
Member

Trott commented Dec 15, 2018

Landed in 87006be...b520254

@Trott Trott closed this Dec 15, 2018
Trott pushed a commit to Trott/io.js that referenced this pull request Dec 15, 2018
WPT covers standards in both W3C and WHATWG, as such it would be
strange to make this disparity explicit in our file names
(e.g. when testing standards that are solely in W3C, like
performance-timeline). Remove the reference to WHATWG will
also make the file names shorter.

PR-URL: nodejs#24826
Refs: nodejs#24823
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Trott pushed a commit to Trott/io.js that referenced this pull request Dec 15, 2018
This patch:

- Support wildcards(*) in WPT runner name matching (needed by e.g.
  encoding where all the tests requires i18n support in the build)
- Print failure reasons when encountering an expected failure
- Fix a bug in copyGlobalsFromObject (previously it copies
  properties from `global` instead of the given `obj`)

Previously an expected failure is printed as

```
[EXPECTED_FAILURE] response.formData() with input: %61+%4d%4D=
```

Now it is printed as

```
[EXPECTED_FAILURE] response.formData() with input: %61+%4d%4D=
missing Request and Response
```

PR-URL: nodejs#24826
Refs: nodejs#24823
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
BethGriggs pushed a commit that referenced this pull request Dec 18, 2018
WPT covers standards in both W3C and WHATWG, as such it would be
strange to make this disparity explicit in our file names
(e.g. when testing standards that are solely in W3C, like
performance-timeline). Remove the reference to WHATWG will
also make the file names shorter.

PR-URL: #24826
Refs: #24823
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
BethGriggs pushed a commit that referenced this pull request Dec 18, 2018
This patch:

- Support wildcards(*) in WPT runner name matching (needed by e.g.
  encoding where all the tests requires i18n support in the build)
- Print failure reasons when encountering an expected failure
- Fix a bug in copyGlobalsFromObject (previously it copies
  properties from `global` instead of the given `obj`)

Previously an expected failure is printed as

```
[EXPECTED_FAILURE] response.formData() with input: %61+%4d%4D=
```

Now it is printed as

```
[EXPECTED_FAILURE] response.formData() with input: %61+%4d%4D=
missing Request and Response
```

PR-URL: #24826
Refs: #24823
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
@BethGriggs BethGriggs mentioned this pull request Dec 18, 2018
sagitsofan pushed a commit to sagitsofan/node that referenced this pull request Dec 24, 2018
WPT covers standards in both W3C and WHATWG, as such it would be
strange to make this disparity explicit in our file names
(e.g. when testing standards that are solely in W3C, like
performance-timeline). Remove the reference to WHATWG will
also make the file names shorter.

PR-URL: nodejs#24826
Refs: nodejs#24823
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
sagitsofan pushed a commit to sagitsofan/node that referenced this pull request Dec 24, 2018
This patch:

- Support wildcards(*) in WPT runner name matching (needed by e.g.
  encoding where all the tests requires i18n support in the build)
- Print failure reasons when encountering an expected failure
- Fix a bug in copyGlobalsFromObject (previously it copies
  properties from `global` instead of the given `obj`)

Previously an expected failure is printed as

```
[EXPECTED_FAILURE] response.formData() with input: %61+%4d%4D=
```

Now it is printed as

```
[EXPECTED_FAILURE] response.formData() with input: %61+%4d%4D=
missing Request and Response
```

PR-URL: nodejs#24826
Refs: nodejs#24823
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
WPT covers standards in both W3C and WHATWG, as such it would be
strange to make this disparity explicit in our file names
(e.g. when testing standards that are solely in W3C, like
performance-timeline). Remove the reference to WHATWG will
also make the file names shorter.

PR-URL: nodejs#24826
Refs: nodejs#24823
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
This patch:

- Support wildcards(*) in WPT runner name matching (needed by e.g.
  encoding where all the tests requires i18n support in the build)
- Print failure reasons when encountering an expected failure
- Fix a bug in copyGlobalsFromObject (previously it copies
  properties from `global` instead of the given `obj`)

Previously an expected failure is printed as

```
[EXPECTED_FAILURE] response.formData() with input: %61+%4d%4D=
```

Now it is printed as

```
[EXPECTED_FAILURE] response.formData() with input: %61+%4d%4D=
missing Request and Response
```

PR-URL: nodejs#24826
Refs: nodejs#24823
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants