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

Allow passing stability options through command line (Fixes #9877) #9945

Merged
merged 2 commits into from Mar 19, 2018

Conversation

Projects
None yet
4 participants
@marimeireles
Copy link
Member

marimeireles commented Mar 9, 2018

repeat_loop = --verify-repeat-loop
repeat_restart = --verify-repeat-restart
(disable) chaos_mode = --verify-no-chaos-mode
max_time = --verify-max-time
(disable) output_results = --verify-no-output-results


This change is Reviewable

Allow passing stability options through command line
repeat_loop = --verify-repeat-loop
repeat_restart = --verify-repeat-restart
(disable) chaos_mode = --verify-no-chaos-mode
max_time = --verify-max-time
(disable) output_results = --verify-no-output-results

@wpt-pr-bot wpt-pr-bot added the infra label Mar 9, 2018

@wpt-pr-bot wpt-pr-bot requested review from gsnedders and jgraham Mar 9, 2018

@w3c-bots

This comment has been minimized.

Copy link

w3c-bots commented Mar 9, 2018

Build PASSED

Started: 2018-03-19 02:12:12
Finished: 2018-03-19 02:24:55

View more information about this build on:

@marimeireles

This comment has been minimized.

Copy link
Member Author

marimeireles commented Mar 10, 2018

I'm not sure if the help messages I wrote are good.
Could you give me some insight on how to make them better if they're not good enough?
Thanks!

default=5,
help="Restart the whole process",
type=int)
mode_group.add_argument("--verify-no-chaos-mode", action="store_false",

This comment has been minimized.

Copy link
@jgraham

jgraham Mar 12, 2018

Contributor

I think this should use dest="verify_chaos_mode" since a True value means that chaos mode is used. Possibly there should also be a --verify-chaos-mode option so that you can specify a value even if we change the default.

This comment has been minimized.

Copy link
@marimeireles

marimeireles Mar 14, 2018

Author Member

Let me see if I get this right you want me create a new --verify-chaos-mode and set both --verify-chaos-mode and --verify-no-chaos-mode with the same dest="verify_chaos_mode"? So that you can both enable and disable from the command line.

This comment has been minimized.

Copy link
@marimeireles

marimeireles Mar 15, 2018

Author Member

Hey @jgraham could you confirm this? I'm not sure if that was what you meant. Thanks!

This comment has been minimized.

Copy link
@jgraham

jgraham Mar 15, 2018

Contributor

Ah apologies, I missed the question. Yes, your understanding is perfect.

help="Disable chaos mode when running on Firefox")
mode_group.add_argument("--verify-max-time", action="store",
default=None,
help="The maximum number of minutes for each test to run",

This comment has been minimized.

Copy link
@jgraham

jgraham Mar 12, 2018

Contributor

This isn't a time in minutes, it's in seconds. And also the time is the maximum time for the job to run, it's not a time per test.

This comment has been minimized.

Copy link
@marimeireles

marimeireles Mar 14, 2018

Author Member

I've set it to a time in minutes:
https://github.com/w3c/web-platform-tests/blob/82da4d2663627032f6befd1a9618de51f31a2509/tools/wptrunner/wptrunner/wptcommandline.py#L100
Would you rather I change it to seconds?

Also, I'll fix the help string.

This comment has been minimized.

Copy link
@jgraham

jgraham Mar 14, 2018

Contributor

Oh. Let's try this way first.

default=None,
help="The maximum number of minutes for each test to run",
type=lambda x: timedelta(minutes=float(x)))
mode_group.add_argument("--verify-no-output-results", action="store_false",

This comment has been minimized.

Copy link
@jgraham

jgraham Mar 12, 2018

Contributor

This should also use dest so that the variable name matches the semantics.

type=int)
mode_group.add_argument("--verify-repeat-restart", action="store",
default=5,
help="Restart the whole process",

This comment has been minimized.

Copy link
@jgraham

jgraham Mar 12, 2018

Contributor

"Number of iterations, for a run that restarts the runner between each iteration"

@@ -82,6 +83,24 @@ def create_parser(product_choices=None):
mode_group.add_argument("--verify-log-full", action="store_true",
default=False,
help="Output per-iteration test results when running verify")
mode_group.add_argument("--verify-repeat-loop", action="store",
default=10,
help="Run tests in a loop for a number of times",

This comment has been minimized.

Copy link
@jgraham

jgraham Mar 12, 2018

Contributor

"Number of iterations for a run that reloads each test without restart."

@marimeireles

This comment has been minimized.

Copy link
Member Author

marimeireles commented Mar 19, 2018

I believe I adressed all the comments @jgraham.

@jgraham
Copy link
Contributor

jgraham left a comment

Thanks!

@jgraham jgraham merged commit 9b3bb05 into web-platform-tests:master Mar 19, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.