872164 - Support in-tree mozharness configs

Status: RESOLVED FIXED

(Testing :: Mochitest, defect)

Description

[:Felipe Gomes (needinfo for replies!)]

I want to be able to use the existing extra functionality of runtests.py in a tryserver push. Personally, i'm mostly interested in setting --test-path, --repeat, and --shuffle, which is only possible locally (or by requesting a machine from releng), which makes it much harder to chase and verify tests problems that cannot be reproduced locally.

I imagine a good approach would be to support pushing a custom testConfig.js file, which is somewhat separate from runtests.py but end up as the same thing (at least in mochitests). Bonus points for that is to being able to keep using the testsuite-targets locally instead of manually running runtests.py.


Additionally, a single test vs. a full suite is too coarse, so I want to also support pushing a custom test manifest override, hopefully part of this same work.
(Test manifest support for mochitests will come from bug 868158).


I'm willing to implement this if I can get some guidance on how to properly do it!

Comment 1

[Jonathan Griffin (:jgriffin)]

I think what we'd want to do here is to support in-tree mozharness configs.  If an in-tree one exists, use it, otherwise, use the default mozharness config.  This is related to a discussion we had in bug 872007.

We decided to do nothing there, since we didn't strongly need that feature for B2G, but it would allow what you're asking (possibly with some modifications to the existing mozharness script).

Comment 2

[Andrew Halberstadt [:ahal]]

I think this is a good idea.

We actually just had a discussion about this this morning, though for a different reason (bug 872007). We decided against in-tree mozharness configs simply because our use case didn't warrant it on its own. But you raise a second good use case and I think we could solve both of them in one fell swoop.

For context, the arguments are provided by mozharness scripts like this: http://mxr.mozilla.org/build/source/mozharness/configs/unittests/linux_unittest.py#44

So I think the easiest way to solve this problem is to:
1) Check in a mozharness config file into the tree (maybe even one per harness?)
2) Mozharness scripts read this config file, overwriting defaults
3) Change mozharness scripts to get all harness parameters from config

Comment 3

[:Felipe Gomes (needinfo for replies!)]

(mid-air with Andrew but posting my comment anyway since I don't really know what I'm talking about!)

Thanks for the info. Can you give me a link of one example mozharness config file? I want it to be very simple, basically a json (that can be read by http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/chrome-harness.js#351) and a list of test files/folders. My main interest is having these working for mochitest-[plain,chrome,browser]. Are mozharness configs something marionette-specific?


Also probably important is that I think the default settings should still be used and these would override only the settings that one wants to change (to avoid having to know the default settings and replicating them in the local file), similar to how mozconfig-extra works.


In my ideal world these would be a file or two that I can stick in topsrcdir and be picked up both when running locally and when they are pushed to try.

Comment 4

[Andrew Halberstadt [:ahal]]

Mozharness is an abstraction around buildbot that releng wrote to make creating and configuring new test jobs easier. It has nothing to do with marionette (other than it is used to run marionette jobs). There is an example config in comment 2.

Just to be clear, the options you want to set in chrome-harness.js are all passed through from the python harness right? The advantage of using mozharness configs is that we are a) setting the values at the source and b) can specify other arbitrary info that mozharness needs to know on a per branch basis. If we go this route I can take this bug as I was planning on working on bug 855893 soon anyway.

Comment 5

[Aki Sasaki (not active)]

I'm not entirely sure you want the full config in-tree.
It's doable.
It might be easier to have a config in mozharness that points to a location to get test configuration from.

Comment 6

[:Felipe Gomes (needinfo for replies!)]

(In reply to Andrew Halberstadt [:ahal] from comment #4)
> Just to be clear, the options you want to set in chrome-harness.js are all
> passed through from the python harness right?

Yes, I think!. I've only been looking at this test harness code recently but that's my current understanding of it. The way that I use it locally is doing the following:

TEST_PATH=browser/base/foobar/ EXTRA_TEST_ARGS="--repeat=2" make mochitest-browser-chrome


Thanks for possibly picking it up! Two questions that I have about the approach you're planning:
- will the configs be suite-specific? I think it needs to be because it doesn't make much sense to specify a custom test-path to any other suite other than the one you're interested.

- will I able to then use this config file automatically both when running locally and when pushed to try? (so that e.g. I don't need to pass TEST_PATH in the command line anymore if the custom config file exists?)


I mentioned to jmaher that I also wanted to hack something simpler directly into chrome-harness to support multiple-tests until we get tests manifest support. But as long as I can pass a custom test-path I think what I'm planning should work. That's for another bug though..

Comment 7

[Andrew Halberstadt [:ahal]]

(In reply to Aki Sasaki [:aki] from comment #5)
> I'm not entirely sure you want the full config in-tree.
> It's doable.
> It might be easier to have a config in mozharness that points to a location
> to get test configuration from.

No, I wasn't planning to put the full config in-tree. My thinking is that only test harness arguments and metadata that differs per branch (like the puppetagain switch) will be in tree. The mozharness scripts will need to be responsible for updating their configurations with the in-tree config.

Though it might be helpful to allow overriding all non in-tree config options so that we can make use of try..

Comment 8

[Andrew Halberstadt [:ahal]]

(In reply to :Felipe Gomes from comment #6)
> - will the configs be suite-specific? I think it needs to be because it
> doesn't make much sense to specify a custom test-path to any other suite
> other than the one you're interested.

Yes the configs will be suite-specific. Either there will be a separate config file that lives with each harness, or one config file that will be set up like:
{ 'mochitest_options': { '--test-path': ... },
  'reftest_options': { ... }
}

> - will I able to then use this config file automatically both when running
> locally and when pushed to try? (so that e.g. I don't need to pass TEST_PATH
> in the command line anymore if the custom config file exists?)

This will get picked up automatically in try, but not locally (unless you use mozharness to run locally, which is do-able but mozharness wasn't really designed for it). However when running locally you should be able to pass in whatever arguments you want into the thing you are running with (e.g mach, make target, runtests.py etc).

Comment 9

[Andrew Halberstadt [:ahal]]

I've been working on this a bit and I've come to a few conclusions:

1) We can't easily store harness options in-tree without giving up the ability to specify them on a per platform basis. Having a config file for each platform/harness I think is a little too complex.

2) There is a trade off between user friendliness and maintainability. For example I could make convenient shortcuts like "reftest_test_manifest", "mochitest_shuffle", etc.. But doing this means each of the mozharness scripts needs to know about them. This can and will get out of hand really quickly.

I think the best thing to do here is:
* have a single mozharness_config.py file checked in to the tree. 
* this file will be mostly empty (except rare cases where branch specific configs are needed)
* mozharness will just load whatever is in there, overriding current values

This would:
a) give us a place to maintain branch specific settings
b) allow people to push custom configurations to try

The downside is that users need to know what the mozharness config keys are (can mitigate this with links to various examples in comments). Another downside is that you won't be able to specify e.g two separate test_paths on two separate platforms.

The upside is that this is by far the simplest and cleanest solution.

Does this make sense to everyone?

Comment 10

[(not currently active) Ted Mielczarek]

This sounds like a significant improvement from what we've got right now. Ideally I'd actually like to see the default options migrate into these files, so that mozharness doesn't have to know the right invocations for all of our harnesses, it can just blindly use whatever we feed it in the tree. This way changing harness options wouldn't be so painful.

Comment 11

[Jonathan Griffin (:jgriffin)]

I like this approach too.  I don't think the downsides are too significant.  If people want different test paths in different platforms on try, they can just submit two separate try jobs, for example.

Comment 12

[Andrew Halberstadt [:ahal]]

Attachment: Patch 1.0 (mozharness) - read config from the tests.zip, factor b2g options into config file

I looked at the marionette options too, but there are so many conditionals for all the various jobs/platforms that moving them into config files would get very messy..

Comment 13

[Andrew Halberstadt [:ahal]]

Attachment: Patch 1.0 (gecko) - add empty mozharness_config.py

I put it in testing/config.. though if you think there is a better place, let me know.

Comment 14

[Andrew Halberstadt [:ahal]]

I tested this on Ash. The latest push has these two exact patches applied:
https://tbpl.mozilla.org/?tree=Ash&rev=b8e0d30f94d7

Comment 15

[Aki Sasaki (not active)]

Comment on attachment 751056 [details] [diff] [review]
Patch 1.0 (mozharness) - read config from the tests.zip, factor b2g options into config file

We shouldn't ever override self.config like this.

We can, however, build something like a self.test_config that we populate from self.config and the in-tree config.

Comment 16

[Andrew Halberstadt [:ahal]]

(In reply to Aki Sasaki [:aki] from comment #15)
> Comment on attachment 751056 [details] [diff] [review]
> Patch 1.0 (mozharness) - read config from the tests.zip, factor b2g options
> into config file
> 
> We shouldn't ever override self.config like this.
> 
> We can, however, build something like a self.test_config that we populate
> from self.config and the in-tree config.

I can do this, but do you mind if I ask why? Yes I could do:
self.test_config = self.config.copy()
self.test_config.update(tree_config)

and then s/self.config/self.test_config in the mozharness script. But now self.config is no longer an accurate representation of the world model and no longer serves any purpose (that I can think of).

Anyway, I'll upload a new patch.

Comment 17

[Aki Sasaki (not active)]

(In reply to Andrew Halberstadt [:ahal] from comment #16)
> (In reply to Aki Sasaki [:aki] from comment #15)
> > Comment on attachment 751056 [details] [diff] [review]
> > Patch 1.0 (mozharness) - read config from the tests.zip, factor b2g options
> > into config file
> > 
> > We shouldn't ever override self.config like this.
> > 
> > We can, however, build something like a self.test_config that we populate
> > from self.config and the in-tree config.
> 
> I can do this, but do you mind if I ask why? Yes I could do:
> self.test_config = self.config.copy()

deepcopy. And we might as well only copy the test portions.
To make it easier, you could self.config['test_config'] = { ... } so you only have to copy the one value.

Overriding anything but test-specific values will only cause confusion, since you can't override config items for actions that have happened already without re-launching the script.

> self.test_config.update(tree_config)
> 
> and then s/self.config/self.test_config in the mozharness script. But now
> self.config is no longer an accurate representation of the world model and
> no longer serves any purpose (that I can think of).

self.config is used by everything.
http://escapewindow.dreamwidth.org/223757.html has one writeup of why; this is a principle feature of mozharness.

Basically, I dealt with scripts elsewhere where config[key] was 4 different values at different times in the script, and I had to deal with timing issues, as well as the fact that these values changed without notice in the log.  So I could verify til I was blue in the face that I was setting the correct value; but since I was doing it at the wrong *time* the buggy line was still getting the wrong config value.

Comment 18

[(not currently active) Ted Mielczarek]

Comment on attachment 751058 [details] [diff] [review]
Patch 1.0 (gecko) - add empty mozharness_config.py

Maybe bikeshedding a bit, but I think making these live next to the harnesses would be clearest, so in testing/mochitest etc.

Comment 19

[Andrew Halberstadt [:ahal]]

(In reply to Aki Sasaki [:aki] from comment #17)
> Basically, I dealt with scripts elsewhere where config[key] was 4 different
> values at different times in the script, and I had to deal with timing
> issues, as well as the fact that these values changed without notice in the
> log.  So I could verify til I was blue in the face that I was setting the
> correct value; but since I was doing it at the wrong *time* the buggy line
> was still getting the wrong config value.

Ok, this is fair and makes sense to me. Though you still kind of have the same problem. It's just that instead of making sure you are checking the config at the right place, you need to make sure you are checking the "right config" (one of which isn't write-locked anyway).

If static configs are a core principal of mozharness, would it be possible to somehow get buildbot to read an in-tree config and pass it in when initially invoking mozharness?

> Overriding anything but test-specific values will only cause confusion,
> since you can't override config items for actions that have happened already
> without re-launching the script.

I agree it is confusing, but it isn't any less confusing than only copying test-specific values. In both cases the outcome from a developers point of view is the same - an option was added and nothing happened.

Comment 20

[Aki Sasaki (not active)]

To clarify: I'm assuming we only want to override test configs, which is probably what we want in most cases.

It's beginning to sound like you want |script -c original_config_file -c http://hgweb/url/to/in/tree/config|, which would populate self.config at the beginning of the script.

Comment 21

[Andrew Halberstadt [:ahal]]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #18)
> Comment on attachment 751058 [details] [diff] [review]
> Patch 1.0 (gecko) - add empty mozharness_config.py
> 
> Maybe bikeshedding a bit, but I think making these live next to the
> harnesses would be clearest, so in testing/mochitest etc.

Originally my idea was to just have a generic mozharness_config.py that could be used for many different purposes. Pushing test configurations to try would have been one use case, bug 855893 would be another. The advantages were that this solution would be both powerful and simple.

But now it sounds like people want some kind of specialized mechanism that is catered towards configuring test harnesses and only works with certain explicitly defined values. This is fine, though I think it'll be more complicated.

Comment 22

[Andrew Halberstadt [:ahal]]

(In reply to Aki Sasaki [:aki] from comment #20)
> To clarify: I'm assuming we only want to override test configs, which is
> probably what we want in most cases.
> 
> It's beginning to sound like you want |script -c original_config_file -c
> http://hgweb/url/to/in/tree/config|, which would populate self.config at the
> beginning of the script.

Your assumption is probably valid. I was just trying to make something generic for the sake of simplicity.

Comment 23

[Aki Sasaki (not active)]

Here's what I did for talos.json:
http://hg.mozilla.org/projects/cedar/file/default/testing/talos/talos.json
and these three methods:
http://hg.mozilla.org/build/mozharness/file/742d5e783f64/mozharness/mozilla/testing/talos.py#l148

Comment 24

[Andrew Halberstadt [:ahal]]

Attachment: Patch 2.0 (mozharness) - use separate tree_config

Made a few adjustments based on feedback.

1. There is now a separate tree_config. Basically you can store anything you want in it, but it is up to the mozharness script to decide to use/not use the values in it. So far with this patch, only values named "<suite>_options" have any affect. Though you can now easily make a mozharness script look at self.tree_config for other values.

2. There is only one file, as opposed to several living with each of the harnesses. I agree with Ted that putting these files next to the harnesses is easier to find for developers. But if we did this, there would still need to be a global mozharness_config.py for config options that affect all test suites. Then you would need to think about options overriding each other between the two locations, the tree config would change depending on the suite being run (this goes against Aki's principle in comment 17) and in general things become more confusing when following along in the logs. Therefore I think a single mozharness_config.py in-tree is the best way to go for now.

Note: The gecko patch doesn't change, but I'll update the comment slightly to reflect that only certain config options will have an affect.

Comment 25

[Aki Sasaki (not active)]

Comment on attachment 752752 [details] [diff] [review]
Patch 2.0 (mozharness) - use separate tree_config

>+        if os.path.isfile(tree_config_path):

At some point we may want to require this file to exist for certain test suites.  We can allow for that now, or if/when we want to make that change.

I imagine it'll just be an

    elif self.config.get('require_tree_config'):
        self.fatal("Missing required tree config file %s!" % tree_config_path)

>+            self.tree_config.update(parse_config_file(tree_config_path))
>+            self.dump_config(file_path=os.path.join(dirs['abs_log_dir'], 'treeconfig.json'),
>+                             config=self.tree_config)

You can do this, or you can self.copy_to_upload_dir(tree_config_path) .
I'm fine either way.
We might want to use abs_upload_dir instead of abs_log_dir, in case we ever start uploading from test jobs.

>+        self.tree_config.lock()

Purely optional depending on how paranoid you are about self.tree_config, but nice.


Thanks for revisiting this patch!
I've thought of a couple more reasons why the first approach can be problematic, which we can cover in the next mozharness meeting (today or in 2 weeks), but it's not urgent by any means.

Comment 26

[Andrew Halberstadt [:ahal]]

Weird.. all the desktop unittests on ash are getting a MemoryError (apparently this means out of memory):
https://tbpl.mozilla.org/php/getParsedLog.php?id=23265108&tree=Ash&full=1

The traceback shows it's happening in my patch.. but this makes no sense to me.

Comment 27

[Aki Sasaki (not active)]

I'm not sure either.
Maybe add some debugging self.info()s to hopefully give us some sense of what's happening?

Comment 28

[Jonathan Griffin (:jgriffin)]

This code is resulting in an infinite loop:

+            if options:
+                for option in options:
                     options.append(option % str_format_values)

Comment 29

[Jonathan Griffin (:jgriffin)]

Comment on attachment 751058 [details] [diff] [review]
Patch 1.0 (gecko) - add empty mozharness_config.py

Review of attachment 751058 [details] [diff] [review]:
-----------------------------------------------------------------

The format of the file looks fine, but I agree with Ted that this file will be more discoverable and require less special knowledge if it lives side-by-side each of the test harnesses.  I'd also name it something like tbpl_config.py, since many developers don't know what mozharness is, but everyone knows what TBPL is.

Comment 30

[Gregory Szorc [:gps]]

Let's name it automation.py.

Comment 31

[Andrew Halberstadt [:ahal]]

(In reply to Jonathan Griffin (:jgriffin) from comment #28)
> This code is resulting in an infinite loop:
> 
> +            if options:
> +                for option in options:
>                      options.append(option % str_format_values)

D'oh!

(In reply to Jonathan Griffin (:jgriffin) from comment #29)
> The format of the file looks fine, but I agree with Ted that this file will
> be more discoverable and require less special knowledge if it lives
> side-by-side each of the test harnesses.  I'd also name it something like
> tbpl_config.py, since many developers don't know what mozharness is, but
> everyone knows what TBPL is.

Did you see point 2 in comment 24? If we did this:
* there would be 5+ of these files (1 per harness plus a global one)
* they would overwrite each other depending on which one got loaded first
* the tree config would change more than once throughout the setup process which violates Aki's principle of mozharness he described in comment 17

Comment 32

[Jonathan Griffin (:jgriffin)]

Comment on attachment 751058 [details] [diff] [review]
Patch 1.0 (gecko) - add empty mozharness_config.py

Review of attachment 751058 [details] [diff] [review]:
-----------------------------------------------------------------

Talked about this with ahal; agreed at this point that either method has potential benefits and drawbacks.  We'll try ahal's method and revisit later if needed.

Comment 33

[Andrew Halberstadt [:ahal]]

https://hg.mozilla.org/build/mozharness/rev/d6cba8407a87
https://hg.mozilla.org/integration/mozilla-inbound/rev/f13114e40916

I'm leaving this open for now as I still need to land it on b2g18.

Felipe, this isn't live quite yet on the mozharness side, but when it is you should be able to add a mochitest_options value to testing/config/mozharness_config.py. The value will need to be similar to this: https://hg.mozilla.org/build/mozharness/file/d6cba8407a87/configs/unittests/linux_unittest.py#l44

For example if you want to use a custom test manifest you should be able to add --manifest <path_to_manifest> to it. To get your custom manifest copied over to the test bundle, you can check it in and add it to http://hg.mozilla.org/mozilla-central/file/4de82dd4b7b6/testing/mochitest/Makefile.in#l33

Let me know if anything isn't working.

Comment 34

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/f13114e40916

Comment 35

[:Felipe Gomes (needinfo for replies!)]

Ahal++, thanks a lot for working on this! I'll use it soon and report back :)

Comment 36

[Andrew Halberstadt [:ahal]]

Pushed to b2g18 branches.

https://hg.mozilla.org/releases/mozilla-b2g18/rev/b2dbbdda1b4d
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/914e95bff3d4

Comment 37

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Andrew Halberstadt [:ahal] from comment #33)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/f13114e40916

This should have received build peer review.

Comment 38

[Andrew Halberstadt [:ahal]]

Sorry, I was under the impression that test packaging/testsuite-targets.mk fell under the testing module.

I'll happily push a follow up if ted or gps or someone wants me to.

Comment 39

[(not currently active) Ted Mielczarek]

Comment on attachment 751058 [details] [diff] [review]
Patch 1.0 (gecko) - add empty mozharness_config.py

Review of attachment 751058 [details] [diff] [review]:
-----------------------------------------------------------------

Post-hoc review here.

::: testing/config/Makefile.in
@@ +15,5 @@
> +  $(NULL)
> +
> +_DEST_DIR = $(DEPTH)/_tests/config
> +libs:: $(CONFIG_FILES)
> +	$(PYTHON) $(topsrcdir)/config/nsinstall.py $^ $(_DEST_DIR)

You don't need this, it doesn't actually do anything useful since you only need to get it in the test package. In fact you could just drop this whole Makefile and put the file copy rule inline in testsuite-targets.mk.

Comment 40

[Andrew Halberstadt [:ahal]]

IRC review=ted

https://hg.mozilla.org/integration/mozilla-inbound/rev/d9d7b08f731e

Comment 41

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d9d7b08f731e

Comment 42

[Aki Sasaki (not active)]

Merged to mozharness production.