Closed Bug 872164 Opened 6 years ago Closed 6 years ago

Support in-tree mozharness configs

Categories

(Testing :: Mochitest, defect)

defect
Not set

Tracking

(b2g18 fixed, b2g18-v1.0.1 fixed)

RESOLVED FIXED
mozilla24
Tracking Status
b2g18 --- fixed
b2g18-v1.0.1 --- fixed

People

(Reporter: Felipe, Assigned: ahal)

References

Details

Attachments

(2 files, 1 obsolete file)

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!
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).
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
(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.
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.
Assignee: nobody → ahalberstadt
Summary: Support pushing a custom test config and manifest to try → Support in-tree mozharness configs
Blocks: 855893
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.
(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..
(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..
(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).
Status: NEW → ASSIGNED
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?
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.
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.
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..
Attachment #751056 - Flags: review?(aki)
I put it in testing/config.. though if you think there is a better place, let me know.
Attachment #751058 - Flags: review?(jgriffin)
I tested this on Ash. The latest push has these two exact patches applied:
https://tbpl.mozilla.org/?tree=Ash&rev=b8e0d30f94d7
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.
Attachment #751056 - Flags: review?(aki) → review-
(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.
(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 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.
(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.
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.
(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.
(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.
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.
Attachment #751056 - Attachment is obsolete: true
Attachment #752752 - Flags: review?(aki)
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.
Attachment #752752 - Flags: review?(aki) → review+
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.
I'm not sure either.
Maybe add some debugging self.info()s to hopefully give us some sense of what's happening?
This code is resulting in an infinite loop:

+            if options:
+                for option in options:
                     options.append(option % str_format_values)
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.
Attachment #751058 - Flags: review?(jgriffin) → review-
Let's name it automation.py.
(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 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.
Attachment #751058 - Flags: review- → review+
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.
Whiteboard: [leave-open]
Ahal++, thanks a lot for working on this! I'll use it soon and report back :)
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
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [leave-open]
(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.
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 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.
IRC review=ted

https://hg.mozilla.org/integration/mozilla-inbound/rev/d9d7b08f731e
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
https://hg.mozilla.org/mozilla-central/rev/d9d7b08f731e
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Merged to mozharness production.
Depends on: 888118
You need to log in before you can comment on or make changes to this bug.