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

Status: RESOLVED FIXED

(Release Engineering :: Applications: MozharnessCore, defect)

Description

[Armen [:armenzg]]

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?

Comment 1

[Bob Owen (:bobowen) (PTO until 17th Oct)]

Attachment: 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.

Comment 2

[Armen [:armenzg]]

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!

Comment 3

[Armen [:armenzg]]

(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.

Comment 4

[Armen [:armenzg]]

The results look good.
Want me to land the patch on your behalf? Or do you want to go ahead?

Comment 5

[Bob Owen (:bobowen) (PTO until 17th Oct)]

(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. :) )

Comment 6

[Armen [:armenzg]]

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.

Comment 7

[Armen [:armenzg]]

Attachment: 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

Comment 8

[Bob Owen (:bobowen) (PTO until 17th Oct)]

(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

[Chris Cooper [:coop] (he/him)]

Merged to production, and deployed.

Comment 10

[Jordan Lund (:jlund)]

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?

Comment 11

[Armen [:armenzg]]

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?

Comment 13

[Armen [:armenzg]]

Comment on attachment 8498910 [details] [diff] [review]
mozharness_tree_config.diff

I think we're fine.

Comment 14

[Armen [:armenzg]]

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.

Comment 15

[Jordan Lund (:jlund)]

(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 :)