Closed Bug 1193264 Opened 4 years ago Closed 4 years ago

Allow saving and using preset try strings from |mach try|

Categories

(Testing :: General, defect)

defect
Not set

Tracking

(firefox44 fixed)

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: jgraham, Unassigned)

References

Details

Attachments

(1 file)

e.g. ./mach try -b d -p linux,linux64 -u mochitest --save mochitest-linux-debug

./mach try --preset mochitest-linux-debug
Bug 1193264 - Add support for saving and reusing try strings in mach try

Adds --save and --preset arguments that can be used to store and reuse
frequently used try strings.
Attachment #8646454 - Flags: review?(cmanchester)
Comment on attachment 8646454 [details]
MozReview Request: Bug 1193264 - Add support for saving and reusing try strings in mach try

Bug 1193264 - Add support for saving and reusing try strings in mach try

Adds --save and --preset arguments that can be used to store and reuse
frequently used try strings.
Comment on attachment 8646454 [details]
MozReview Request: Bug 1193264 - Add support for saving and reusing try strings in mach try

Bug 1193264 - Add support for saving and reusing try strings in mach try

Adds --save and --preset arguments that can be used to store and reuse
frequently used try strings.
Comment on attachment 8646454 [details]
MozReview Request: Bug 1193264 - Add support for saving and reusing try strings in mach try

https://reviewboard.mozilla.org/r/15777/#review15787

::: testing/mach_commands.py:397
(Diff revision 3)
> +            for x in item.split(","):
> +                if x.strip():

[x for x in item.split(",") if x.strip()] ?

::: testing/mach_commands.py:403
(Diff revision 3)
> +        if not len(kwargs["paths"]) and not kwargs["tests"]:
> +            print("Paths or tests must be specified as an argument to autotry.")

This didn't get an update for the option to only provide tags.

::: testing/mach_commands.py:435
(Diff revision 3)
> -    def autotry(self, builds=None, platforms=None, paths=None, verbose=None,
> +    def autotry(self, **kwargs):
> -                push=None, tags=None, tests=None, extra_args=None):

I would prefer to keep individual parameters.

::: testing/mach_commands.py:481
(Diff revision 3)
> +            defaults = at.load_config(kwargs["_load"])
> +
> +            if defaults is None:
> +                print("No saved configuration called %s found in mach.ini" % kwargs["_load"],
> +                      file=sys.stderr)

kwargs["load"]

::: testing/tools/autotry/autotry.py:32
(Diff revision 3)
> +    parser.add_argument('--save', dest="save", action='store',
> +                        help="Save the command line arguments for future use with --preset")
> +    parser.add_argument('--preset', dest="load", action='store',
> +                        help="Load a saved set of arguments. Additional arguments will override saved ones")

A nice feature for a follow up would be to have the final result of the last N invocations of mach try saved automatically, with a way to dump those to the console and select by index.

::: testing/tools/autotry/autotry.py:32
(Diff revision 3)
> +    parser.add_argument('--save', dest="save", action='store',
> +                        help="Save the command line arguments for future use with --preset")
> +    parser.add_argument('--preset', dest="load", action='store',
> +                        help="Load a saved set of arguments. Additional arguments will override saved ones")

A nice feature for a follow up would be to have the final result of the last N invocations of mach try saved automatically, with a way to dump those to the console and select by index.

::: testing/tools/autotry/autotry.py:82
(Diff revision 3)
> +        config_file = os.path.join(self.topsrcdir, "mach.ini")

I think we can use mach_context.state_dir for this.

::: testing/tools/autotry/autotry.py:95
(Diff revision 3)
> +        for item in kwargs.keys():
> +            if item.startswith("_"):
> +                kwargs.pop(item)

I don't think this underscore prefix is relevant anymore.
Attachment #8646454 - Flags: review?(cmanchester)
Comment on attachment 8646454 [details]
MozReview Request: Bug 1193264 - Add support for saving and reusing try strings in mach try

Bug 1193264 - Add support for saving and reusing try strings in mach try

Adds --save and --preset arguments that can be used to store and reuse
frequently used try strings.
Attachment #8646454 - Flags: review?(cmanchester)
Blocks: 1204120
Attachment #8646454 - Flags: review?(cmanchester)
Comment on attachment 8646454 [details]
MozReview Request: Bug 1193264 - Add support for saving and reusing try strings in mach try

https://reviewboard.mozilla.org/r/15777/#review17109

This doesn't look ready for re-review.
https://reviewboard.mozilla.org/r/15777/#review15787

> [x for x in item.split(",") if x.strip()] ?

Fixed, in the sense that I suddenly realised that I actually use the test[platform] syntax, so I implemented that too.

> I would prefer to keep individual parameters.

Unfortunately that doesn't work with the --load feature; it needs a dictionary of arguments that can be updated. Of course I could make this work with explicitly named arguments, but it would involve a lot of repetitive code, so I don't think it's worthwhile.
Attachment #8646454 - Flags: review?(cmanchester)
Comment on attachment 8646454 [details]
MozReview Request: Bug 1193264 - Add support for saving and reusing try strings in mach try

Bug 1193264 - Add support for saving and reusing try strings in mach try

Adds --save and --preset arguments that can be used to store and reuse
frequently used try strings.
https://reviewboard.mozilla.org/r/15777/#review15787

> Unfortunately that doesn't work with the --load feature; it needs a dictionary of arguments that can be updated. Of course I could make this work with explicitly named arguments, but it would involve a lot of repetitive code, so I don't think it's worthwhile.

There are several values to provide as defaults -- updating a dictionary is hardly the only reasonable implementation, but I guess this is ok.
Comment on attachment 8646454 [details]
MozReview Request: Bug 1193264 - Add support for saving and reusing try strings in mach try

https://reviewboard.mozilla.org/r/15777/#review17321

::: testing/mach_commands.py:517
(Diff revision 5)
> +                print("No saved configuration called %s found in mach.ini" % kwargs["load"],

autotry.ini now

::: testing/tools/autotry/autotry.py:63
(Diff revision 5)
> +class TryArgumentParser(object):
> +    """Simple three-state parser for handling expressions
> +    of the from "foo[sub item, another], bar,baz". This takes
> +    input from the TryArgumentTokenizer and runs through a small
> +    state machine, returning a dictionary of {top-level-item:[sub_items]}
> +    i.e. the above would result in
> +    {"foo":["sub item", "another"], "bar": [], "baz": []}
> +    In the case of invalid input a ValueError is raised."""

Reinventing the try parser here really seems like overkill -- where are we actually interacting with individual platform filters? Couldn't we just serialize the result of parsing suites/filters in a well known format and convert it to try syntax only when pushing?
Attachment #8646454 - Flags: review?(cmanchester)
Comment on attachment 8646454 [details]
MozReview Request: Bug 1193264 - Add support for saving and reusing try strings in mach try

https://reviewboard.mozilla.org/r/15777/#review17333

::: testing/tools/autotry/autotry.py:101
(Diff revision 5)
> +            raise ValueError("Error parsing try string, unexpected" % (self.token[0]))

There's no format pattern in this string.

::: testing/tools/autotry/autotry.py:203
(Diff revision 5)
> +        kwargs = vars(parser().parse_args(data.split()))

"parser" undefined?
Attachment #8646454 - Flags: review+
https://reviewboard.mozilla.org/r/15777/#review17333

> "parser" undefined?

No, but I tried to make the name more obvious.
Comment on attachment 8646454 [details]
MozReview Request: Bug 1193264 - Add support for saving and reusing try strings in mach try

Bug 1193264 - Add support for saving and reusing try strings in mach try

Adds --save and --preset arguments that can be used to store and reuse
frequently used try strings.
backed out for the test bustage in bug 1193215
https://hg.mozilla.org/mozilla-central/rev/3b7594b3f88b
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in before you can comment on or make changes to this bug.