Bug 1272113 is moving AWSY into the tree. It is important that developers also be able to run the tests locally via a mach command.

copy of corresponding patch from bug 1272113

copy of corresponding patch from bug 1272113

copy of corresponding patch from bug 1272113

This adjusts the platforms for which awsy will run as discussed in bug 1272113 so we can run this on any linux{32,64} build.

Bug 1344813 - prepare awsy for mach command. * parse_about_memory.py - remove extraneous data variable. * test_memory_usage.py - get webRootDir, resultsDir from testvars to support mach command. - standardize directories. - catch exceptions thrown by marionette when closing tabs. * awsy_script.py - change to use webRootDir, resultsDir and to pass to test_memory_usage.py in additional testvars.json file. - standardize directories. - set strict=False in StructuredOutputParser in order to prevent ascii encoding errors due to marionette loading csdn.net and outputing non-ascii to stdout.

This implements the mach command for use in a checked out tree. Usage: ./mach awsy-test [options...] --web-root WEBROOTDIR Path to web server root directory. --results RESULTSDIR Path to results directory. --entities ENTITIES Number of urls to load. --max-tabs MAXTABS Maximum number of tabs to open. --iterations ITERATIONS Number of times to run through the test suite. --per-tab-pause PERTABPAUSE Seconds to wait in between opening tabs. --settle-wait-time SETTLEWAITTIME Seconds to wait for things to settled down.

The mach command runs well for me locally. I ran a number of try runs using the full set of urls and default values: https://treeherder.mozilla.org/#/jobs?repo=try&author=bclary@mozilla.com&fromchange=f688f52901416e83e710d94315ee384c41576419&tochange=ffaf5b549fce06343ac0a8d3d3e96e74b7bbfc83 The early ones suffer from the ascii encoding issues, but the last 3 are green. The actual tree was not changed in between the runs, so the performance values should be close to identical and they are. pyang: To get this to work with your patches I needed these additional patches. adjust-test-platforms.yml.patch: To add linux64 builds etc so the tests would run with a try build. awsy-fix.patch: This changes the use of directories so that we would not rely on the current working directory as well as allow us to specify the directories when running via a mach command. mach-commands.patch: To add the in tree version of mach-commands.py. jmaher: This in-tree version of the mach command doesn't use mozharness. I used the in-tree version of tooltool.py. If we want to add a version of the mach command that runs from the test packages, we'll probably have to use mozharness and also install tooltool.py in a virtualenv but I've only begun looking into that. Let me know what you think.

Comment on attachment 8846618 [details] [diff] [review] mach-commands.patch Review of attachment 8846618 [details] [diff] [review]: ----------------------------------------------------------------- this is a great start, my comments are more of a newbie viewpoint to using AWSY. I am excited to not use mozharness- I think the future is simple mach interface to a runner script that we use locally and in automation. That is still far off though. ::: testing/awsy/mach_commands.py @@ +30,5 @@ > + from mozlog.structured import commandline > + > + parser = MarionetteArguments() > + > + parser.add_argument('--web-root', action='store', type=str, dest='webRootDir', this will be confusing for people to setup originally, can we have a |mach awsy setup| mode that downloads and installs tp5n and sets an environment variable? or maybe it looks in a common directory ~/.mozilla/testing/tp5n @@ +32,5 @@ > + parser = MarionetteArguments() > + > + parser.add_argument('--web-root', action='store', type=str, dest='webRootDir', > + help='Path to web server root directory.') > + parser.add_argument('--results', action='store', type=str, dest='resultsDir', will these results be understandable to humans? If not a follow up to ensure there is a wiki or toolchain to understand the results. @@ +34,5 @@ > + parser.add_argument('--web-root', action='store', type=str, dest='webRootDir', > + help='Path to web server root directory.') > + parser.add_argument('--results', action='store', type=str, dest='resultsDir', > + help='Path to results directory.') > + parser.add_argument('--entities', action='store', type=int, dest='entities', this isn't clear why we would adjust this? what are the default values? this would apply to the rest of the items in this list.

(In reply to Joel Maher ( :jmaher) from comment #8) > Comment on attachment 8846618 [details] [diff] [review] > mach-commands.patch > > Review of attachment 8846618 [details] [diff] [review]: > ----------------------------------------------------------------- > > this is a great start, my comments are more of a newbie viewpoint to using > AWSY. I am excited to not use mozharness- I think the future is simple mach > interface to a runner script that we use locally and in automation. That is > still far off though. > Oh, we don't want to use mozharness? I was going to model the mach_test_package_commands.py off of marionette's which does use mozharness a bit... > ::: testing/awsy/mach_commands.py > @@ +30,5 @@ > > + from mozlog.structured import commandline > > + > > + parser = MarionetteArguments() > > + > > + parser.add_argument('--web-root', action='store', type=str, dest='webRootDir', > > this will be confusing for people to setup originally, can we have a |mach > awsy setup| mode that downloads and installs tp5n and sets an environment > variable? or maybe it looks in a common directory ~/.mozilla/testing/tp5n > Actually this is a bit of over specification. If you don't specify it, it will default to a reasonable value of your topobjdir/_tests/awsy/html and tp5n will be automatically installed in topobjdir/_tests/awsy/html/page_load_test. The tooltool_cache is placed in topsrcdir. tp5n.zip will be automatically downloaded and will be checked on each use to see if it needs to be updated. I clean out the resultDir each time, but need to clean out the page_load_test if tp5n.zip changes. The mach_test_package_commands.py will be somewhat different in the location it chooses by default. As I mentioned earlier I was thinking that this would give people the freedom to point the web root anywhere. If I also added an option for the manifest file containing urls, people would have complete freedom when developing or testing other page sets. I don't see why we couldn't have an mach awsy-setup command, though I don't think it will be needed in general. We just need to be mindful that whatever we do for mach will need to be compatible with the taskclsuter/mozharness stuff. > @@ +32,5 @@ > > + parser = MarionetteArguments() > > + > > + parser.add_argument('--web-root', action='store', type=str, dest='webRootDir', > > + help='Path to web server root directory.') > > + parser.add_argument('--results', action='store', type=str, dest='resultsDir', > > will these results be understandable to humans? If not a follow up to > ensure there is a wiki or toolchain to understand the results. > > @@ +34,5 @@ > > + parser.add_argument('--web-root', action='store', type=str, dest='webRootDir', > > + help='Path to web server root directory.') > > + parser.add_argument('--results', action='store', type=str, dest='resultsDir', > > + help='Path to results directory.') > > + parser.add_argument('--entities', action='store', type=int, dest='entities', > > this isn't clear why we would adjust this? what are the default values? > this would apply to the rest of the items in this list. In normal usage you wouldn't. I mostly put them in since I could and it seemed like a good idea to be able to control from the command line. For example, when running quick tests which I didn't want to run for a full hour, I could set the values to reduce the number of pages loaded, the wait times etc. Very helpful when hacking the mach command itself. The default values are defined in the awsy module __init__.py. I initially specified the defaults in the options, but removed it since it would be easy to get the two out of sync. I'll think about how to present this better. I think we'll need to add this kind of documentation to the tree similarly to web-platform, mozharness, marionette, and mozbase. Eric has a bunch already we can repurpose. Maybe a mach command for that!

A '--quick' option that sets the proper config values for a quick test might be nice (I have this in atsy), something like: - settleWaitTime: 1 - perTabPause: 1 - iterations: 1 - entities: 3

yeah, I don't think we need a setup command, it seems as though the web-root stuff is for everything and defaults to a good place. What if you want to edit the pages and see if the memory is reduced in local runs? Could we do that? I like the idea of --quick mode- overall this is shaping up well!

Mercurial auto-merge the patch with https://hg.mozilla.org/mozilla-central/annotate/1bce56d3f45d/taskcluster/ci/test/test-platforms.yml#l30 I'll fix this later, sorry for disturbing.

(In reply to Joel Maher ( :jmaher) from comment #11) > What if you want to edit the pages and see if the memory is reduced in local runs? Could we do that? Yes. You can specify the --web-root and --results options to point to a different set of pages and a different location to store the results. With the addition of --page-manifest you could also have a differently named file instead of tp5n.manifest. I'll need to add logic to not deal with downloading the t5n.zip from tooltool.

Try run with all of the patches applied: https://treeherder.mozilla.org/#/jobs?repo=try&revision=570bc02f3d3e245746d85fc77eb6b77f6fae642c

Comment on attachment 8846616 [details] [diff] [review] awsy-fix.patch Review of attachment 8846616 [details] [diff] [review]: ----------------------------------------------------------------- a lot of little changes, thanks for the nice commit message.

Comment on attachment 8849624 [details] [diff] [review] mach-commands.patch v2 Review of attachment 8849624 [details] [diff] [review]: ----------------------------------------------------------------- overall this looks good- where do the results end up at?

Comment on attachment 8849627 [details] [diff] [review] mach-awsy-remove-template.patch Review of attachment 8849627 [details] [diff] [review]: ----------------------------------------------------------------- I like getting rid of more hardcoded list of urls :)

(In reply to Joel Maher ( :jmaher) from comment #17) > Comment on attachment 8849624 [details] [diff] [review] > mach-commands.patch v2 > > Review of attachment 8849624 [details] [diff] [review]: > ----------------------------------------------------------------- > > overall this looks good- where do the results end up at? In the results directory.

Comment on attachment 8849624 [details] [diff] [review] mach-commands.patch v2 Review of attachment 8849624 [details] [diff] [review]: ----------------------------------------------------------------- Looks pretty good! I just did a local run on my macbook :) Made a few comments, mainly I think lumping this in with marionette tests might not be the clearest way to go for an end-user. Also the commit message is a bit off -- I wish it were that simple! One other thing we might consider, possibly as a follow up, is to add an option to spit out the perf data in a human readable format rather than a json blob. It would also be useful to let the user know where they can find the memory reports after the run completes, something like: > "Memory reports saved to obj-x86_64-apple-darwin16.4.0-debug.noindex/_tests/awsy/results/" ::: testing/awsy/awsy/test_memory_usage.py @@ +61,5 @@ > + if "pageManifest" in self.testvars and self.testvars["pageManifest"]: > + tp5n_manifest = self.testvars["pageManifest"] > + else: > + tp5n_manifest = os.path.join(self._webroot_dir, 'page_load_test', 'tp5n', > + 'tp5n.manifest') maybe something like the following would be more clear: > default_tp5n_manifest = os.path.join(...) > tp5n_manifest = self.testvars.get("pageManifest", default_tp5n_manifest) ::: testing/awsy/mach_commands.py @@ +22,5 @@ > +) > + > +def is_firefox(cls): > + """Must have Firefox build.""" > + return conditions.is_firefox(cls) Does this really work? I tried passing in the path to a pre-built firefox nightly and it failed, ie: > ./mach awsy-test --quick --binary ../atsy-test/FirefoxNightly.app/Contents/MacOS/firefox @@ +34,5 @@ > + if AWSY_PATH not in sys.path: > + sys.path.append(AWSY_PATH) > + from awsy import ITERATIONS, PER_TAB_PAUSE, SETTLE_WAIT_TIME, MAX_TABS > + > + parser = MarionetteArguments( Do we really gain anything by reusing MarionetteArguments? It seems like all our options get lost in the mix, even with the '(AWSY)' prefixing. Or maybe it's possible to lump these arguments into a subgroup. @@ +45,5 @@ > + specific arguments are marked with (AWSY) below. awsy-test > + will automatically download the tp5n.zip talos pageset from > + tooltool and install it under topobjdir/_tests/awsy/html. You > + can specify your own page set by specifying --web-root and > + --page-manifest. ''')) This could probably use some line breaks, it's a bit hard to parse. Also if we don't piggy-back on `MarionetteArguments` we can get rid of some of the text. @@ +96,5 @@ > + ) > + > + parser = setup_awsy_argument_parser() > + > + if not tests: Nice, so we could pass in a different test file? @@ +178,5 @@ > + return 1 > + else: > + return 0 > + > + @Command('awsy-test', category='testing', I don't see 'awsy-test' listed in |mach help|, do we have to add that manually?

Comment on attachment 8846616 [details] [diff] [review] awsy-fix.patch thanks, look good to me.

Comment on attachment 8849627 [details] [diff] [review] mach-awsy-remove-template.patch lgtm :)

Comment on attachment 8849624 [details] [diff] [review] mach-commands.patch v2 I'm ok with this patch. One little thing, for wrap function of condition.is_firefox, maybe uses import conditions.is_firefox as is_firefox can use is_firefox in both line 184 and 195. But I don't really know the intention so only for suggestion.

(In reply to Eric Rahm [:erahm] from comment #20) > Comment on attachment 8849624 [details] [diff] [review] > mach-commands.patch v2 > > Review of attachment 8849624 [details] [diff] [review]: > ----------------------------------------------------------------- > > Looks pretty good! I just did a local run on my macbook :) > > Made a few comments, mainly I think lumping this in with marionette tests > might not be the clearest way to go for an end-user. Also the commit message > is a bit off -- I wish it were that simple! > Yeah, I did that to make it simpler but it does hide the real usage message. > One other thing we might consider, possibly as a follow up, is to add an > option to spit out the perf data in a human readable format rather than a > json blob. > That sounds like the right idea. > It would also be useful to let the user know where they can find the memory > reports after the run completes, something like: > > > "Memory reports saved to obj-x86_64-apple-darwin16.4.0-debug.noindex/_tests/awsy/results/" > Both you and Joel mentioned that. Ok. > ::: testing/awsy/awsy/test_memory_usage.py > @@ +61,5 @@ > > + if "pageManifest" in self.testvars and self.testvars["pageManifest"]: > > + tp5n_manifest = self.testvars["pageManifest"] > > + else: > > + tp5n_manifest = os.path.join(self._webroot_dir, 'page_load_test', 'tp5n', > > + 'tp5n.manifest') > > maybe something like the following would be more clear: > > > default_tp5n_manifest = os.path.join(...) > > tp5n_manifest = self.testvars.get("pageManifest", default_tp5n_manifest) > Looks nice. > ::: testing/awsy/mach_commands.py > @@ +22,5 @@ > > +) > > + > > +def is_firefox(cls): > > + """Must have Firefox build.""" > > + return conditions.is_firefox(cls) > > Does this really work? I tried passing in the path to a pre-built firefox > nightly and it failed, ie: > > > ./mach awsy-test --quick --binary ../atsy-test/FirefoxNightly.app/Contents/MacOS/firefox > Works with my local build. I'll look into it. Haven't tested the binary argument. > @@ +34,5 @@ > > + if AWSY_PATH not in sys.path: > > + sys.path.append(AWSY_PATH) > > + from awsy import ITERATIONS, PER_TAB_PAUSE, SETTLE_WAIT_TIME, MAX_TABS > > + > > + parser = MarionetteArguments( > > Do we really gain anything by reusing MarionetteArguments? It seems like all > our options get lost in the mix, even with the '(AWSY)' prefixing. > We get the flexibility of having the full set of marionette arguments available. It is reasonable to have them since we really are a marionette test. I wonder if an argument to enable marionette arguments would be useful/understandable? The question is if the default values from the parser are necessary. I can look at it more. For example, by default we *wouldn't* show the full marionette arguments. > Or maybe it's possible to lump these arguments into a subgroup. > I'll look into that more. > @@ +45,5 @@ > > + specific arguments are marked with (AWSY) below. awsy-test > > + will automatically download the tp5n.zip talos pageset from > > + tooltool and install it under topobjdir/_tests/awsy/html. You > > + can specify your own page set by specifying --web-root and > > + --page-manifest. ''')) > > This could probably use some line breaks, it's a bit hard to parse. Also if > we don't piggy-back on `MarionetteArguments` we can get rid of some of the > text. > That was my original thought but I can't use paragraphs in the text using the formatter. It just munged them all together regardless. I've seen examples on the web where people have subclassed the formatter to preserve the line breaks but the internals of the class are definitely labelled private. We could that with the proviso that if we ever move to python3, this would break. > @@ +96,5 @@ > > + ) > > + > > + parser = setup_awsy_argument_parser() > > + > > + if not tests: > > Nice, so we could pass in a different test file? > I believe so as long as it was a marionette based test. Let me try that explicitly. > @@ +178,5 @@ > > + return 1 > > + else: > > + return 0 > > + > > + @Command('awsy-test', category='testing', > > I don't see 'awsy-test' listed in |mach help|, do we have to add that > manually? I do see it actually in my local tree. Right under Testing: Run tests. awsy-test .... If it is not there, perhaps I am missing something in the patch that I have locally here.

fyi, found the solution for the grouping issue. still working on the other issues + nits.

I believe I have a patch which addresses everyone's concerns though the binary issue is still outstanding. I can run with a build in my workstation on Linux but I fail to detect the binary on OSX. Looking into that now.

try run https://treeherder.mozilla.org/#/jobs?repo=try&author=bclary@mozilla.com&fromchange=a75bf51a377a42c43d2ec27a0af9201eadc7666a&exclusion_profile=false&selectedJob=85727652

Notes: test_memory_usage.py: * used Eric's suggestion which is much nicer. * formatted the perf blob when written to disk which makes it more human readable. * output a note stating where the blob was written to help find the results. awsy/mach_commands.py: * removed the is_firefox calls altogether. I found it did not work on osx and was not necessary. You can either specify the MOZCONFIG (or use the default if you didn't specify one during the build) or use --binary. * moved the awsy command arguments to the MachCommands class decorating run_awsy_test. This added the AWSY group which clearly delineates the AWSY arguments. * added a doc string to run_awsy_test which turns out does what I wanted with the formatted description. testing/mach_commands.py: * removed the awsy-test since the new decorators did the same thing.

Comment on attachment 8850182 [details] [diff] [review] mach-commands.patch v3 Review of attachment 8850182 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, tested it out locally on Linux. I think I found another follow up issue, it seems to take 30 seconds or so to shutdown the webservers: > 0:12.64 LOG: MainThread INFO tearing down webservers! > 0:46.32 LOG: MainThread INFO processing data in /home/erahm/dev/mozilla-central/obj-x86_64-pc-linux-gnu-clang-release/_tests/awsy/result Maybe that's just expected behavior.

Comment on attachment 8850184 [details] [diff] [review] mach-awsy-remove-template.patch v2 Review of attachment 8850184 [details] [diff] [review]: ----------------------------------------------------------------- Nice cleanup.

(In reply to Eric Rahm [:erahm] from comment #30) > Maybe that's just expected behavior. For the full configuration, it is shutting down 100 web servers which might be the cause, but I think it also takes a bit too long even on the quick run. We might be starting up more than the required number if we don't do a full test. I'll look into it more.

Comment on attachment 8850184 [details] [diff] [review] mach-awsy-remove-template.patch v2 Review of attachment 8850184 [details] [diff] [review]: ----------------------------------------------------------------- looks good.

Comment on attachment 8850182 [details] [diff] [review] mach-commands.patch v3 Review of attachment 8850182 [details] [diff] [review]: ----------------------------------------------------------------- Thanks!

We defaulted to always starting 100 web servers regardless of the settings or the number of urls in the manifest. This patch uses the actual number of urls or the entities if set. try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=03fbbf7201c13ceb4f5eb701089f50f98823f618

Comment on attachment 8850532 [details] [diff] [review] mach-webservers.patch Review of attachment 8850532 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, looks good.

Comment on attachment 8850532 [details] [diff] [review] mach-webservers.patch Review of attachment 8850532 [details] [diff] [review]: ----------------------------------------------------------------- looks good!

Comment on attachment 8850182 [details] [diff] [review] mach-commands.patch v3 Review of attachment 8850182 [details] [diff] [review]: ----------------------------------------------------------------- The patch looks good.

Comment on attachment 8850532 [details] [diff] [review] mach-webservers.patch Review of attachment 8850532 [details] [diff] [review]: ----------------------------------------------------------------- nice patch.

Pushed by bclary@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/7178c9d489d1 prepare awsy for mach command, r=jmaher,pyang. https://hg.mozilla.org/integration/mozilla-inbound/rev/cd933fa335b7 Implement AWSY as a marionette-based mach command, r=jmaher,pyang,erahm. https://hg.mozilla.org/integration/mozilla-inbound/rev/9a2657d53f6e remove template urls from awsy, r=jmaher,pyang,erahm. https://hg.mozilla.org/integration/mozilla-inbound/rev/f5affb1a24ca Only start as many web servers as required, r=jmaher,pyang,erahm.

https://hg.mozilla.org/mozilla-central/rev/7178c9d489d1 https://hg.mozilla.org/mozilla-central/rev/cd933fa335b7 https://hg.mozilla.org/mozilla-central/rev/9a2657d53f6e https://hg.mozilla.org/mozilla-central/rev/f5affb1a24ca

https://hg.mozilla.org/releases/mozilla-beta/rev/cc0c7b6cf342 https://hg.mozilla.org/releases/mozilla-beta/rev/43d6189761c5 https://hg.mozilla.org/releases/mozilla-beta/rev/9d537df8223a https://hg.mozilla.org/releases/mozilla-beta/rev/b87d6bc4300e