If download-and-extract is not run then we don't call _read_tree_config and can't run-tests

RESOLVED FIXED

Status

Release Engineering
Mozharness
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: armenzg, Assigned: bobowen)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [easier-mozharness])

Attachments

(2 attachments)

(Reporter)

Description

3 years ago
On a related note I also tried using the --run-tests option that you mentioned on IRC.

At first it was complaining that there were no mochitest_options in the in-tree config, this was because the in-tree config is loaded in the download_and_extract step, which hasn't been run.

So, I added the following to the start of preflight_run_tests (in testbase.py):
        # If the in tree config hasn't been loaded by a previous step, load it here.
        if len(self.tree_config) == 0:
            self._read_tree_config()

this seemed to do the trick.
I also had to specify a --binary-path, because this is set up by the install step and without this it was just looking in the build directory.

You still have to pass --installer-url and --test-url (it moans if you don't with developer_config.py specified).
The installer one seems to be used to get to the other downloaded files.
The test one doesn't seem to be used, because I could pass anything.

Do you want me to create a bug and upload my patch to testbase.py for the in-tree config load?
(Reporter)

Updated

3 years ago
Summary: Make desktop_unittest.py to read in-tree configs if they have not been set → Make desktop_unittest.py to read in-tree configs if they have not been set through download-and-extract
(Assignee)

Comment 1

3 years ago
Created attachment 8498804 [details] [diff] [review]
Load in-tree config in run-tests preflight step, if is hasn't been loaded previously.

Here's my patch for this.
I'm not sure if other steps can be run in isolation and whether the in-tree config is needed for them.
If, that is the case then maybe a more general fix would be better.
Assignee: nobody → bobowencode
Status: NEW → ASSIGNED
Attachment #8498804 - Flags: review?(armenzg)
(Reporter)

Comment 2

3 years ago
Comment on attachment 8498804 [details] [diff] [review]
Load in-tree config in run-tests preflight step, if is hasn't been loaded previously.

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

The patch should be good.
However, I would like to see it run in automation before landing it:
https://tbpl.mozilla.org/?tree=Ash&jobname=mochitest-1&rev=7e1b38694052

I have re-triggered some of those mochitest-1 jobs to get a feeling of what to expect.

Thanks Bob!
Attachment #8498804 - Flags: review?(armenzg) → review+
(Reporter)

Comment 3

3 years ago
(In reply to Bob Owen (:bobowen) from comment #1)
> Created attachment 8498804 [details] [diff] [review]
> Load in-tree config in run-tests preflight step, if is hasn't been loaded
> previously.
> 
> Here's my patch for this.
> I'm not sure if other steps can be run in isolation and whether the in-tree
> config is needed for them.
> If, that is the case then maybe a more general fix would be better.

For now, we will have to grow a feeling of where the current state of affairs is.
I recently started developing more in mozharness and I'm trying running steps by steps and adding more conditions.
(Reporter)

Comment 4

3 years ago
The results look good.
Want me to land the patch on your behalf? Or do you want to go ahead?
(Assignee)

Comment 5

3 years ago
(In reply to Armen Zambrano - Automation & Tools Engineer (:armenzg) from comment #4)
> The results look good.
> Want me to land the patch on your behalf? Or do you want to go ahead?

Unfortunately I don't have access yet, so can you land for me please.
(I must get round to getting higher commit access, in fact I'll do it now. :) )
(Reporter)

Comment 6

3 years ago
Comment on attachment 8498804 [details] [diff] [review]
Load in-tree config in run-tests preflight step, if is hasn't been loaded previously.

Landed on default:
https://hg.mozilla.org/build/mozharness/rev/fe6c15736684

Thank you for your help!
I will wait until Buildduty merges to the production branch (to be run on tbpl.mozilla.org) just to be sure we don't regress anything.
Attachment #8498804 - Flags: checked-in+
(Reporter)

Comment 7

3 years ago
Created attachment 8498910 [details] [diff] [review]
mozharness_tree_config.diff

Jordan we added in here:
https://hg.mozilla.org/build/mozharness/rev/fe6c15736684

However, I get the feeling that we should also add the same condition as inside of download_and_extract().

What do you think?

(Meanwhile I trigger a talos job on Ash to confirm my suspicion).
https://tbpl.mozilla.org/?tree=Ash&jobname=talos&rev=7e1b38694052
Attachment #8498910 - Flags: review?(jlund)
(Assignee)

Comment 8

3 years ago
(In reply to Armen Zambrano - Automation & Tools Engineer (:armenzg) from comment #7)

> However, I get the feeling that we should also add the same condition as
> inside of download_and_extract().

Probably best to be safe, but I think we'll be OK because _read_tree_config only tries to load the config if in_tree_config exists in self.config.

Comment 9

3 years ago
Merged to production, and deployed.
Comment on attachment 8498910 [details] [diff] [review]
mozharness_tree_config.diff

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

::: mozharness/mozilla/testing/testbase.py
@@ +568,5 @@
>  
>      def preflight_run_tests(self):
>          """preflight commands for all tests"""
>          # If the in tree config hasn't been loaded by a previous step, load it here.
> +        if self.test_url and len(self.tree_config) == 0:

why do we need to have the test_url? once we are at 'run_tests' action, wouldn't we already have the test zip downloaded and extracted?

@@ -568,5 @@
>  
>      def preflight_run_tests(self):
>          """preflight commands for all tests"""
>          # If the in tree config hasn't been loaded by a previous step, load it here.
> -        if len(self.tree_config) == 0:

traditionally, the idiom in mozharness has always been to:

# attr setter
def query_tree_config(self):
    if self.tree_config:
        return self.tree_config
    self.tree_config = dict(parse_config_file(c['path/to/tree_config'])
    return self.tree_config

# within any action
def action_foo(self):
    tree_config = self.query_tree_config()


this allows us to use self.tree_config within any action and not worry if it has been initialized appropriately, dependent on another part of the script, or require repeated conditions. can that be applied here?
Attachment #8498910 - Flags: review?(jlund) → review+
(Reporter)

Comment 11

3 years ago
Comment on attachment 8498910 [details] [diff] [review]
mozharness_tree_config.diff

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

::: mozharness/mozilla/testing/testbase.py
@@ +568,5 @@
>  
>      def preflight_run_tests(self):
>          """preflight commands for all tests"""
>          # If the in tree config hasn't been loaded by a previous step, load it here.
> +        if self.test_url and len(self.tree_config) == 0:

I am concerned with jobs like talos where there is no test zip to download.
In fact, I think running talos as a developer would fail without this if we only called --run-tests.
It is also the same thing we check for inside of download_and_extract().

@@ -568,5 @@
>  
>      def preflight_run_tests(self):
>          """preflight commands for all tests"""
>          # If the in tree config hasn't been loaded by a previous step, load it here.
> -        if len(self.tree_config) == 0:

Would you prefer us to switch to this approach?
(Reporter)

Updated

3 years ago
Duplicate of this bug: 1049750
(Reporter)

Updated

3 years ago
Summary: Make desktop_unittest.py to read in-tree configs if they have not been set through download-and-extract → If download-and-extract is not run then we don't call _read_tree_config and can't run-tests
(Reporter)

Comment 13

3 years ago
Comment on attachment 8498910 [details] [diff] [review]
mozharness_tree_config.diff

I think we're fine.
Attachment #8498910 - Flags: review+
(Reporter)

Comment 14

3 years ago
jlund, I believe you want us to rewrite the code with that approach. If you do so, please let us know and I will open a new bug for it.
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
(In reply to Armen Zambrano - Automation & Tools Engineer (:armenzg) from comment #14)
> jlund, I believe you want us to rewrite the code with that approach. If you
> do so, please let us know and I will open a new bug for it.

sorry I tend to go over cc bugmail every 2 days. ni/r?/f? are best for faster responses.

I r+ as I don't want to block so the approach described is not necessary, just a nit. I doubt a follow up bug will ever be addressed. So let's keep this resolved for good assuming it works :)
You need to log in before you can comment on or make changes to this bug.