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

Run only a supported set of WPT test types by default. #26772

Merged
merged 1 commit into from Jun 5, 2020

Conversation

@jdm
Copy link
Member

jdm commented Jun 3, 2020

wptrunner introduced a new test type for print reftests, and by default any unsupported type causes the test runner to report an unexpected error, even if none of those tests are run. These changes avoid similar breakage by limiting the default test types to ones that are supported by our test runners.

@highfive
Copy link

highfive commented Jun 3, 2020

Heads up! This PR modifies the following files:

@jdm
Copy link
Member Author

jdm commented Jun 3, 2020

@highfive highfive assigned Manishearth and unassigned nox Jun 3, 2020
parser.add_argument('--no-default-test-types', default=False, action="store_true",
help="Run the default set of test types provided by wptrunner"),
Comment on lines 85 to 86

This comment has been minimized.

@SimonSapin

SimonSapin Jun 3, 2020

Member

The name and description of this flag seem contradictory

This comment was marked as outdated.

@jdm

jdm Jun 3, 2020

Author Member

Default in this case refers to the test of test types that are selected by mach's test-wpt command when this flag is not present. Is there a better way to describe it?

This comment has been minimized.

@jdm

jdm Jun 3, 2020

Author Member

It's true - the default in the command name refers to the user experience and the flags selected by test-wpt. The default in the description refers to the flags selected by wptrunner without any other input.

This comment has been minimized.

@SimonSapin

SimonSapin Jun 3, 2020

Member

Default in this case refers to […] when this flag is not present.

In that case, the flag’s description "run the default set" is wrong since setting the flag makes a different set be used than the one called default.

Unless the "by wptrunner" (as opposed to mach) means that it intentionally refers to a different set. In this case, it’s not great to use the same word "default" to refer to two different sets in the flag’s name and its description.

@jdm jdm force-pushed the jdm:wpt-test-types branch from 0919344 to da1b77d Jun 3, 2020
@jdm
Copy link
Member Author

jdm commented Jun 3, 2020

@SimonSapin I tried a different description.

@SimonSapin
Copy link
Member

SimonSapin commented Jun 3, 2020

Sorry to nit pick :) This description is better but --no-default-test-types sounds like it runs fewer test types, but it runs more. How about --all-test-types?

Copy link
Member

Manishearth left a comment

r=me, I also prefer all-test-types

@jdm
Copy link
Member Author

jdm commented Jun 3, 2020

So, I like the current argument name because it also allows --no-default-test-types --test-types reftest print-reftest. I find that easier to reason about than --all-test-types --test-types reftest print-reftest.

@Manishearth
Copy link
Member

Manishearth commented Jun 3, 2020

oh! i see. can you mention --test-types in the help, then?

@jdm jdm force-pushed the jdm:wpt-test-types branch from da1b77d to 21b670f Jun 3, 2020
@jdm
Copy link
Member Author

jdm commented Jun 3, 2020

Done.

@SimonSapin
Copy link
Member

SimonSapin commented Jun 3, 2020

Oh I hadn’t realized. Does --no-default-test-types by itself run nothing? If not, could/should we make it so?

@jdm
Copy link
Member Author

jdm commented Jun 3, 2020

No, --no-default-test-types causes the set of all known test types from wptrunner to be run. It can be thought of as enabling the behaviour that currently exists on master.

@jdm
Copy link
Member Author

jdm commented Jun 3, 2020

Personally, as the person who interacts most with wptrunner internals, I am interested in the following use cases:

  • enabling the default behaviour of wptrunner on-demand, but disabled by default because it can break running tests (--no-default-test-types)
  • running an explicit subset of test types on a per-run basis, to test specific harness behaviours (--no-default-test-types --test-types foo bar)
  • running with a known-good configuration on CI and locally so that nobody else has to think about this (``)
@Manishearth
Copy link
Member

Manishearth commented Jun 3, 2020

r=me

@jdm
Copy link
Member Author

jdm commented Jun 5, 2020

@bors-servo r=manishearth

@bors-servo
Copy link
Contributor

bors-servo commented Jun 5, 2020

📌 Commit 21b670f has been approved by manishearth

@bors-servo
Copy link
Contributor

bors-servo commented Jun 5, 2020

Testing commit 21b670f with merge 17ac381...

@bors-servo
Copy link
Contributor

bors-servo commented Jun 5, 2020

☀️ Test successful - status-taskcluster
Approved by: manishearth
Pushing 17ac381 to master...

@bors-servo bors-servo merged commit 17ac381 into servo:master Jun 5, 2020
2 checks passed
2 checks passed
Community-TC (pull_request) TaskGroup: success
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.