Closed
Bug 1075992
Opened 10 years ago
Closed 10 years ago
If download-and-extract is not run then we don't call _read_tree_config and can't run-tests
Categories
(Release Engineering :: Applications: MozharnessCore, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: armenzg, Assigned: bobowen)
References
Details
(Whiteboard: [easier-mozharness])
Attachments
(2 files)
1.50 KB,
patch
|
armenzg
:
review+
armenzg
:
checked-in+
|
Details | Diff | Splinter Review |
1.14 KB,
patch
|
Details | Diff | Splinter Review |
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•10 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•10 years ago
|
||
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.
Reporter | ||
Comment 2•10 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•10 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•10 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•10 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•10 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•10 years ago
|
||
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•10 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•10 years ago
|
||
Merged to production, and deployed.
Comment 10•10 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: 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•10 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•10 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•10 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•10 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
Closed: 10 years ago
Resolution: --- → FIXED
Comment 15•10 years ago
|
||
(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.
Description
•