Closed Bug 1193215 Opened 10 years ago Closed 10 years ago

|mach try| should work with general paths rather than paths to manifests specifically

Categories

(Testing :: General, defect)

Product:
Testing
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox44 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla44
Tracking Flags:
Tracking Status
firefox44 --- fixed

People

(Reporter: jgraham, Unassigned)

References

Details

Attachments

(1 file, 1 obsolete file)

MozReview Request: Bug 1193215 - Support for passing test directories through mach try.
40 bytes, text/x-review-board-request
chmanchester
: review+
Details
For some test types e.g web-platform-tests the granularity of testing by manifest is not sufficient to select a useful subset of tests. Instead we should allow passing in any source tree path and have the harness resolve this into tests.
Bug 1193215 - Support for passing test directories through mach try. This adds support for web-platform-tests to mach try. It changes the implementation so that instead of passing paths to manifests, the user passes arbitary paths in the source tree, and tests under that path are run, with test discovery mainly left to the harness.
Attachment #8646453 - Flags: review?(cmanchester)
Comment on attachment 8646453 [details] MozReview Request: Bug 1193215 - Support for passing test directories through mach try. https://reviewboard.mozilla.org/r/15773/#review14467 How much testing has this seen on try? I'd be interested in seeing b2g and android. ::: testing/mach_commands.py:7 (Diff revision 1) > +import argparse Import appears unused. ::: testing/mach_commands.py:405 (Diff revision 1) > + rv_platforms = [] > + for item in platforms: > + rv_platforms.extend(x for x in item.split(",") if x for item in platforms) These are mostly just re-joined later, but I see this accomodates -p being action=append. Why change to action=append? ::: testing/mach_commands.py:430 (Diff revision 1) > - 'and commands performed.') > - @CommandArgument('-p', dest='platforms', required='AUTOTRY_PLATFORM_HINT' not in os.environ, > + def autotry(self, builds=None, platforms=None, paths=None, _verbose=None, > + _push=None, tags=None, tests=None, extra_args=None): Underscore prefixing isn't helping anything here. ::: testing/mach_commands.py:434 (Diff revision 1) > Pushes the specified tests to try. The simplest way to specify tests is > by using the -u argument, which will behave as usual for try syntax. This help message needs an update with this patch. ::: testing/mach_commands.py:462 (Diff revision 1) > - print("mach try is under development, please file bugs blocking 1149670.") > + if tests is None: Don't remove this. ::: testing/mach_commands.py:470 (Diff revision 1) > - print('ERROR please commit changes before continuing') > - sys.exit(1) > + #print('ERROR please commit changes before continuing') > + #sys.exit(1) > + pass This looks unintentional. ::: testing/mach_commands.py:478 (Diff revision 1) > - manifests_by_flavor = at.resolve_manifests(paths=paths, tags=tags) > + paths = [os.path.relpath(os.path.normpath(os.path.abspath(item)), self.topsrcdir) for item in paths] Is this somehow no longer needed? ::: testing/mach_commands.py:498 (Diff revision 1) > - if verbose: > + print('The following try syntax was calculated:\n%s' % msg) > - print('The following try syntax was calculated:\n\n\t%s\n' % msg) I don't think we need to print the try syntax unconditionally. ::: testing/mozharness/mozharness/mozilla/testing/try_tools.py:57 (Diff revision 1) > - def set_extra_try_arguments(self, known_try_arguments): > + def parse_extra_try_arguments(self, known_try_arguments): This should still be set_extra_try_arguments. ::: testing/mozharness/mozharness/mozilla/testing/try_tools.py:130 (Diff revision 1) > - out_lines = [line for line in lines if filter_fn(line)] > + if args is None: > + return rv I'm guessing a defaultdict(list) is falsy, but returning None here would be more explicit. ::: testing/mozharness/mozharness/mozilla/testing/try_tools.py:133 (Diff revision 1) > - self.rmtree(m) > - self.write_to_file(m, '\n'.join(out_lines)) > + for item in args: > + suite, path = item.split(":", 1) I still think doing this with individual arguments (--mochitest-paths, --reftest-paths, etc.) would be better than parsing within arguments. ::: testing/mozharness/scripts/desktop_unittest.py:379 (Diff revision 1) > + flavor = self._query_try_flavor(suite_category, suite) `flavor` unused. ::: testing/mozharness/scripts/desktop_unittest.py:580 (Diff revision 1) > + if flavor: > + try_options, try_tests = self.try_args(flavor) > + if try_tests: > + tests_list = try_tests > + > + for option in try_options: > + option = option % replace_dict > + cmd.append(option) > + > + if tests_list: > + cmd.append("--") > + cmd.extend(tests_list) We're going to end up with this in all the scripts -- is there a reasonable way to extract it? ::: testing/tools/autotry/autotry.py:16 (Diff revision 1) > TRY_SYNTAX_TMPL = """ > -try: -b %s -p %s -u %s -t none %s %s > +try: -b %s -p %s -u %s -t none %s %s --try-test-paths %s > """ If you're going to stop using this, remove it. ::: testing/tools/autotry/autotry.py:23 (Diff revision 1) > + parser.add_argument('-p', dest='platforms', action="append", > + help='Platforms to run. (required if not found in the environment)') > + parser.add_argument('-u', dest='tests', action="append", > + help='Test suites to run in their entirety') Why change these to action="append"? ::: testing/tools/autotry/autotry.py:52 (Diff revision 1) > - 'crashtest', > - ] > + "path": lambda x: os.path.join("tests", "reftest", "tests", x) > + }, > + 'crashtest': { > + "path": lambda x: os.path.join("tests", "reftest", "tests", x) > + }, It seems like the place to do this path translation is in mozharness. ::: testing/tools/autotry/autotry.py:88 (Diff revision 1) > - manifest = os.path.relpath(t['manifest'], self.topsrcdir) > - manifests_by_flavor[flavor].add(manifest) > - > + for path in sorted_paths: > + if t['file_relpath'].startswith(path): > + paths_by_flavor[flavor].add(path_func(path)) Does it actually matter you sorted these? Did you mean to break out of the loop when you find a match? ::: testing/tools/autotry/autotry.py:94 (Diff revision 1) > + def remove_duplicates(self, paths_by_flavor, tests): > + rv = {} > + for item in paths_by_flavor: > + if item not in tests: > + rv[item] = paths_by_flavor[item].copy() > + return rv You're comparing "flavors" to the arguments to "-u" -- I don't think this is going to work as intended in some cases, because arguments to "-u" are interpreted as suites as they would appear in try syntax. ::: testing/tools/autotry/autotry.py:102 (Diff revision 1) > + #problem: if we specify a directory like foo/bar to reftests and there isn't > + #a reftest file in foo/bar but there is one in foo/bar/baz, the reftest harness > + #won't run the tests I think this comment is slightly less helpful than the one it replaced, I'd settle for getting rid of them both. ::: testing/tools/autotry/autotry.py:117 (Diff revision 1) > + 'web-platform-tests': ['web-platform-tests-1'] Put a comment at the end of this line. ::: testing/tools/autotry/autotry.py:120 (Diff revision 1) > + parts = ["try:", "-b", builds, "-p", ",".join(platforms)] I think this reads easier with the string template than by building up a list. ::: testing/tools/autotry/autotry.py:125 (Diff revision 1) > + if flavor not in suites: This has the same potential issue comparing job names and test flavors. o see
Attachment #8646453 - Flags: review?(cmanchester)
https://reviewboard.mozilla.org/r/15773/#review14467 > These are mostly just re-joined later, but I see this accomodates -p being action=append. Why change to action=append? Because the current syntax is horribly inconsistent. Build options must be one option, unseparated. Platforms and test types must be one option, comma separated. Tags must be multiple options. Paths must be space separated. This adds a little consistency (and makes the command line more compasable) by allowing tags, tests, and platforms to works in the same way. > Underscore prefixing isn't helping anything here. Right, this was left over from an earlier iteration in which it was important to know which arguments ended up as part of the try command. > Don't remove this. OK I guess, but for the record I disagree. *Everything* we do is "under development", or dead, and I think the signal that you get from this output is "don't trust this tool is you are trying to get work done", which I don't think is a reasonable or helpful message to give. > I don't think we need to print the try syntax unconditionally. I think it's helpful because you get to see exactly what the tool did, which should help people trust it. > This should still be set_extra_try_arguments. Is there some larger reason why that I'm missing? "set" seems very generic to me and actively misleading for code that is actually for reading the arguments. > I'm guessing a defaultdict(list) is falsy, but returning None here would be more explicit. It would also complicate the API substantially by making the return type Dict-or-None rather than just Dict, meaning that you couldn't e.g. just iterate over the result without doing an explicit check for None. > I still think doing this with individual arguments (--mochitest-paths, --reftest-paths, etc.) would be better than parsing within arguments. Do you think this strongly enough that it's worth refactoring the implementation? If we used the --[suite]-paths style it would be necessary to know the suite at the time we parse the arguments, which we currently don't because the arguments are parsed once when reading the config, and we don't know the suite until later, particularly for desktop_unittest which can run multiple suites per invocation. Obviously this isn't an insurmountable problem, but I haven't understood the advantages of the other approach that make it worth changing. > We're going to end up with this in all the scripts -- is there a reasonable way to extract it? The problem — and it is a problem — is that there are slight variations in the way each suite builds its command line argument strings. So there are slight variations in the same code for every suite. I agree that's not ideal. > Why change these to action="append"? [see other issue] > It seems like the place to do this path translation is in mozharness. My worry about moving it into the mozharness scripts is that they are per-platform, so it seems easy to make mistakes where one platform is updated but not others. I can just move it into try_tools.py, but that really feels like it doesn't have to know about specific suites, whereas this code already does. Maybe putting it into the scripts is the way to go, but I'd like to confirm that's what you have in mind. > Put a comment at the end of this line. What comment? > I think this reads easier with the string template than by building up a list. The problem with the string template is that it gets messy quickly when some parts are optional.
https://reviewboard.mozilla.org/r/15773/#review14467 > Because the current syntax is horribly inconsistent. Build options must be one option, unseparated. Platforms and test types must be one option, comma separated. Tags must be multiple options. Paths must be space separated. This adds a little consistency (and makes the command line more compasable) by allowing tags, tests, and platforms to works in the same way. The inconsistencies are due to existing implementations of try syntax and tags, but this is adding ways to do things -- tests can now be specified multiple times or comma delimited. > OK I guess, but for the record I disagree. *Everything* we do is "under development", or dead, and I think the signal that you get from this output is "don't trust this tool is you are trying to get work done", which I don't think is a reasonable or helpful message to give. I would agree if I had a little more initial feedback (or any) from someone using the tool. It helps to know something isn't dead, and if you file a bug, it will get fixed. > I think it's helpful because you get to see exactly what the tool did, which should help people trust it. This is going to be a wall of text in some cases, I don't think it's a reasonable default. > Is there some larger reason why that I'm missing? "set" seems very generic to me and actively misleading for code that is actually for reading the arguments. jlund noted reviewing something around here that the observable effect of the function is to set members that are picked up later, I agreed its worth noting in the function name and changed it. > It would also complicate the API substantially by making the return type Dict-or-None rather than just Dict, meaning that you couldn't e.g. just iterate over the result without doing an explicit check for None. Ok, then try_test_paths needs to be initialized to an empty dict. > Do you think this strongly enough that it's worth refactoring the implementation? If we used the --[suite]-paths style it would be necessary to know the suite at the time we parse the arguments, which we currently don't because the arguments are parsed once when reading the config, and we don't know the suite until later, particularly for desktop_unittest which can run multiple suites per invocation. Obviously this isn't an insurmountable problem, but I haven't understood the advantages of the other approach that make it worth changing. I just think it's a bad design. It's probably unlikely to fail in practice so I'm ok with landing this version. > My worry about moving it into the mozharness scripts is that they are per-platform, so it seems easy to make mistakes where one platform is updated but not others. I can just move it into try_tools.py, but that really feels like it doesn't have to know about specific suites, whereas this code already does. Maybe putting it into the scripts is the way to go, but I'd like to confirm that's what you have in mind. I would say move it into try_tools.py, it seems ok for it to have an idea of the suites it applies to. > What comment? s/comment/comma/
Attachment #8646453 - Flags: review?(cmanchester)
Comment on attachment 8646453 [details] MozReview Request: Bug 1193215 - Support for passing test directories through mach try. Bug 1193215 - Support for passing test directories through mach try. This adds support for web-platform-tests to mach try. It changes the implementation so that instead of passing paths to manifests, the user passes arbitary paths in the source tree, and tests under that path are run, with test discovery mainly left to the harness.
https://reviewboard.mozilla.org/r/15773/#review14467 > The inconsistencies are due to existing implementations of try syntax and tags, but this is adding ways to do things -- tests can now be specified multiple times or comma delimited. Yes, but it makes it more consistent overall. You can either remember which things are comma separated and which things are multiple options, or just use multiple options always. > Is this somehow no longer needed? I think this is now done in paths_by_flavor > jlund noted reviewing something around here that the observable effect of the function is to set members that are picked up later, I agreed its worth noting in the function name and changed it. Well my experience was the opposite; I assumed that a method called set_something would have some output effect, when in fact it's all about reading some external state. I don't think it's much of a convention in mozharness either; I see a handful of set_* methods, but they either take an already-read value and set a property (e.g. set_config) or actually set something external to buildbot (set_device_time).
Comment on attachment 8646453 [details] MozReview Request: Bug 1193215 - Support for passing test directories through mach try. https://reviewboard.mozilla.org/r/15773/#review15301 ::: testing/mozharness/mozharness/mozilla/testing/try_tools.py:159 (Diff revision 2) > - if self.try_test_paths.get(flavor): > + if self.try_test_paths[flavor]: Is this going to throw a KeyError for branches other than try?
Attachment #8646453 - Flags: review?(cmanchester)
https://reviewboard.mozilla.org/r/15773/#review14467 > Well my experience was the opposite; I assumed that a method called set_something would have some output effect, when in fact it's all about reading some external state. I don't think it's much of a convention in mozharness either; I see a handful of set_* methods, but they either take an already-read value and set a property (e.g. set_config) or actually set something external to buildbot (set_device_time). I'm not convinced. The change isn't related to the current patch, please don't include it here.
https://reviewboard.mozilla.org/r/15773/#review15313 This is pretty much good to go, but it's a pretty big patch and I'd like to take another look once remaining issues are addressed. ::: testing/mach_commands.py:450 (Diff revisions 1 - 2) > + Tests may be further filtered by passing --tag to the command, > + which will run only tests marked as belonging to the specified > + tags. The new comment doesn't mention the ability to push with just tags as input. ::: testing/tools/autotry/autotry.py:59 (Diff revisions 1 - 2) > + "chrome": "mochitests", > + "browser-chrome": "mochitests", > + "devtools-chrome": "mochitests", As arguments to -u, it might make sense to include mochitest-o, mochitest-bc, and mochitest-dt here.
Comment on attachment 8646453 [details] MozReview Request: Bug 1193215 - Support for passing test directories through mach try. Bug 1193215 - Support for passing test directories through mach try. This adds support for web-platform-tests to mach try. It changes the implementation so that instead of passing paths to manifests, the user passes arbitary paths in the source tree, and tests under that path are run, with test discovery mainly left to the harness.
Attachment #8646453 - Flags: review?(cmanchester)
Comment on attachment 8646453 [details] MozReview Request: Bug 1193215 - Support for passing test directories through mach try. https://reviewboard.mozilla.org/r/15773/#review15773 Cancelling review for now until issues have been addressed.
Attachment #8646453 - Flags: review?(cmanchester)
https://reviewboard.mozilla.org/r/15773/#review15805 ::: testing/tools/autotry/autotry.py:32 (Diff revision 3) > + parser.add_argument('extra_args', nargs=argparse.REMAINDER, > + help='Extra arguments to put in the try push') It may not be a critical feature, but I realize now we've lost the ability to run a directory's worth of tests without running every harness. In the current implementation this can be achieved by specifying -u and paths.
Comment on attachment 8646453 [details] MozReview Request: Bug 1193215 - Support for passing test directories through mach try. Bug 1193215 - Support for passing test directories through mach try. This adds support for web-platform-tests to mach try. It changes the implementation so that instead of passing paths to manifests, the user passes arbitary paths in the source tree, and tests under that path are run, with test discovery mainly left to the harness.
Attachment #8646453 - Flags: review?(cmanchester)
https://reviewboard.mozilla.org/r/15773/#review15313 > The new comment doesn't mention the ability to push with just tags as input. This issue isn't addressed in the most recent patch. > As arguments to -u, it might make sense to include mochitest-o, mochitest-bc, and mochitest-dt here. This issue isn't addressed in the most recent patch.
https://reviewboard.mozilla.org/r/15773/#review14467 I think b2g and android work badly because the way that we pick a chunk to run doesn't allow for non-desktop suites. Actually it also doesn't allow for reftest on non-linux vs reftest-1 and reftest-2 on Windows. Realistically I don't think that trychooser syntax is good enough for our needs here and I think we need to look at basing mach try on mozci asap; bug 1194264 seems like the relevant dependency. OTOH if we can make this work as well as the existing code I don't think this patch should necessarily wait on that functionaily.
Comment on attachment 8646453 [details] MozReview Request: Bug 1193215 - Support for passing test directories through mach try. Bug 1193215 - Support for passing test directories through mach try. This adds support for web-platform-tests to mach try. It changes the implementation so that instead of passing paths to manifests, the user passes arbitary paths in the source tree, and tests under that path are run, with test discovery mainly left to the harness.
Comment on attachment 8646453 [details] MozReview Request: Bug 1193215 - Support for passing test directories through mach try. https://reviewboard.mozilla.org/r/15773/#review17293 Great stuff. ::: testing/tools/autotry/autotry.py:53 (Diff revisions 4 - 5) > + 'mochitest-o': ['mochitest-o'], > + 'mochitest-bc': ['mochitest-browser-chrome-1', > + 'mochitest-e10s-browser-chrome-1'], > + 'mochitest-dt': ['mochitest-dt', > + 'mochitest-e10s-devtools-chrome'], These aren't flavors according to the build system, they don't need to be added here. ::: testing/tools/autotry/autotry.py:64 (Diff revisions 4 - 5) > "mochitest": "mochitests", > "xpcshell": "xpcshell", > "chrome": "mochitests", > "browser-chrome": "mochitests", > "devtools-chrome": "mochitests", > + "mochitest-o": "mochitests", > + "mochitest-bc": "mochitests", > + "mochitest-dt": "mochitests", This is getting sort of complicated, but I think this needs to be: ... "browser-chrome": "mochitest-bc", "devtools-chrome": "mochitest-dt", "chrome": "mochitest-o", ... ::: testing/tools/autotry/autotry.py:124 (Diff revisions 4 - 5) > for test in flavor_tests: > paths.add("%s:%s" % (flavor, test)) I think this introduces a bug: if multiple paths selecting e.g. xpcshell tests were passed and nothing was passed to -u, only the first path would be added, because ```suite not in suites``` would be false after the first time through the loop. If leaving out the "intersection" behavior for now is a lot easier I'm ok with leaving that for a follow up.
Attachment #8646453 - Flags: review?(cmanchester) → review+
https://reviewboard.mozilla.org/r/15773/#review17293 > I think this introduces a bug: if multiple paths selecting e.g. xpcshell tests were passed and nothing was passed to -u, only the first path would be added, because ```suite not in suites``` would be false after the first time through the loop. > > If leaving out the "intersection" behavior for now is a lot easier I'm ok with leaving that for a follow up. Oh, I think this is actually intended, but maybe it means --and doesn't do what you wanted. With --and you only run the tests that are both found under the specified path(s) and selected with -u. So if nothing is selected with -u there are no tests to run anyway. At least all the cases I thought of seemed to work as I expected based on the semantics I had in mind.
https://reviewboard.mozilla.org/r/15773/#review17293 > Oh, I think this is actually intended, but maybe it means --and doesn't do what you wanted. With --and you only run the tests that are both found under the specified path(s) and selected with -u. So if nothing is selected with -u there are no tests to run anyway. At least all the cases I thought of seemed to work as I expected based on the semantics I had in mind. I need to look at this again, now I don't think it introduces the bug I thought it did.
Comment on attachment 8646453 [details] MozReview Request: Bug 1193215 - Support for passing test directories through mach try. Bug 1193215 - Support for passing test directories through mach try. This adds support for web-platform-tests to mach try. It changes the implementation so that instead of passing paths to manifests, the user passes arbitary paths in the source tree, and tests under that path are run, with test discovery mainly left to the harness.
Comment on attachment 8646453 [details] MozReview Request: Bug 1193215 - Support for passing test directories through mach try. https://reviewboard.mozilla.org/r/15773/#review17439 ::: testing/mozharness/scripts/android_emulator_unittest.py:488 (Diff revision 6) > - tests = self.test_suite_definitions[self.test_suite]["tests"] > - elif "tests" in self.config["suite_definitions"][suite_category]: > - tests = self.config["suite_definitions"][suite_category]["tests"] > - > + cmd.extend(self.query_tests_args( > + self.config["suite_definitions"][suite_category].get("tests"), > + self.test_suite_definitions[self.test_suite].get("tests"), > + try_tests)) > - if tests: androidx86_emulator_unittest.py needs an update. The "S4" job runs xpcshell tests, and appears to break with this patch.
Attachment #8646453 - Flags: review+
https://reviewboard.mozilla.org/r/15773/#review17439 > androidx86_emulator_unittest.py needs an update. The "S4" job runs xpcshell tests, and appears to break with this patch. This comment is for a different patch...
https://reviewboard.mozilla.org/r/15773/#review17293 > I need to look at this again, now I don't think it introduces the bug I thought it did. I pulled the branch and did some local testing, this is working as I expect it to.
Comment on attachment 8646453 [details] MozReview Request: Bug 1193215 - Support for passing test directories through mach try. https://reviewboard.mozilla.org/r/15773/#review17625
Attachment #8646453 - Flags: review+
Comment on attachment 8646453 [details] MozReview Request: Bug 1193215 - Support for passing test directories through mach try. Bug 1193215 - Support for passing test directories through mach try. This adds support for web-platform-tests to mach try. It changes the implementation so that instead of passing paths to manifests, the user passes arbitary paths in the source tree, and tests under that path are run, with test discovery mainly left to the harness.
Comment on attachment 8646453 [details] MozReview Request: Bug 1193215 - Support for passing test directories through mach try. Bug 1193215 - Support for passing test directories through mach try. This adds support for web-platform-tests to mach try. It changes the implementation so that instead of passing paths to manifests, the user passes arbitary paths in the source tree, and tests under that path are run, with test discovery mainly left to the harness.
Attachment #8646453 - Flags: review+ → review?(cmanchester)
Attachment #8646453 - Flags: review?(cmanchester) → review+
sorry had to back this out since one of the changes caused https://treeherder.mozilla.org/logviewer.html#?job_id=14640968&repo=mozilla-inbound
Flags: needinfo?(james)
Bleh. New try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b2ded3e67d60
Flags: needinfo?(james)
Very sorry James but had to back this out again for failures like https://treeherder.mozilla.org/logviewer.html#?job_id=14699959&repo=mozilla-inbound
Flags: needinfo?(james)
This thing where I get backed out for testsuites that don't run with "-u all" on Try is rather annoying. I'll file a bug on that android suite. In the meantime I tried -u really-all (i.e. listed every single suite) to see if that helps: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2a5098459aa1
Flags: needinfo?(james)
So eventually adding the job with try extender worked.
https://hg.mozilla.org/mozilla-central/rev/067d9325416a
Status: NEW → RESOLVED
Closed: 10 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Comment on attachment 8667002 [details] MozReview Request: Bug 1193215 - Add a mode to mach test to run impacted tests according to moz.build dependency info. r=ahal Sorry, wrong bug.
Attachment #8667002 - Flags: review?(ahalberstadt)
Depends on: 1352000
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: