Closed Bug 1206415 Opened 4 years ago Closed 4 years ago

move the suite concept into talos itself

Categories

(Testing :: Talos, defect)

defect
Not set

Tracking

(firefox43 affected, firefox44 fixed)

RESOLVED FIXED
mozilla44
Tracking Status
firefox43 --- affected
firefox44 --- fixed

People

(Reporter: parkouss, Assigned: parkouss)

References

Details

Attachments

(1 file)

So we discussed that a bit with jmaher, and I proposed to add a --suite argument into talos, so it will behave a bit more like on harness.

This will allow to:

 - run a suite from the talos command line (obviously)
 - reuse all the arguments from talos cmdline into "mach talos" without adding new code.

This would resolve bug 1192965 and bug 1192964 in one go also.
Oh, also maybe this is the place to discuss that, I'm not sure. I ask the question, should we maintain "./mach talos-test" ? It is pretty easy to just install talos in a virtualenv and run it, and it is quite a bunch of code required for the mach command currently (mach_commands.py and the special stuff in mozharness talos.py).

Another thing we could do is rework on the mach_commands.py file to make it run talos without harness at all.

Well, this can be discussed as a follow up, the patch here anyway keep the the mach command, just allowing to use all talos options.
Bug 1206415 - move the suite concept into talos itself. r=jmaher
Attachment #8663330 - Flags: review?(jmaher)
Assignee: nobody → j.parkouss
Status: NEW → ASSIGNED
Comment on attachment 8663330 [details]
MozReview Request: Bug 1206415 - move the suite concept into talos itself. r=jmaher

https://reviewboard.mozilla.org/r/19771/#review17823

overall this is really nice.  A few possible new bugs to open, maybe some comments to add.  I need to see if this actually runs the exact same tests as before- we have some definitions in buildbot-configs for jobs and their respective tests, but the data in talos.json is definitive and we should ensure that is being used.

::: testing/mozharness/mozharness/mozilla/testing/talos.py:80
(Diff revision 1)
>  

there is probably more room to remove other config options from this file.

::: testing/mozharness/mozharness/mozilla/testing/talos.py:254
(Diff revision 1)
>          if not self.talos_json:

if we are not using talosjson in here, then why not remove this?  Ideally we could just have a comment that says "when X, remove this"

::: testing/mozharness/mozharness/mozilla/testing/talos.py:413
(Diff revision 1)
>              if plugins_url:

plugins- we really don't use these anymore- we should file a bug to cleanup the talos.json and ensure that plugins are not used at all.  These were supposed to be flash plugins, but they are years old- sort of invalid at this point.

::: testing/talos/mach_commands.py:82
(Diff revision 1)
>              'python_webserver': False,

we have python_webserver=False here, but adding --develop forces it to true, can we simplify that a bit?  Maybe just wait until we get the python webserver used in automation?

::: testing/talos/talos/config.py:431
(Diff revision 1)
> +    with open(os.path.join(utils.here, '..', 'talos.json')) as f:

can we do a os.path.dirname or something like that instead of os.path.join(utils.here, '..')
Attachment #8663330 - Flags: review?(jmaher) → review+
Comment on attachment 8663330 [details]
MozReview Request: Bug 1206415 - move the suite concept into talos itself. r=jmaher

Bug 1206415 - move the suite concept into talos itself. r=jmaher
https://reviewboard.mozilla.org/r/19771/#review17823

> there is probably more room to remove other config options from this file.

Well I tried to remove others, but builds failed. :( Maybe we can look at that in a follow up bug ?

> if we are not using talosjson in here, then why not remove this?  Ideally we could just have a comment that says "when X, remove this"

hm, I think it is defined locally, but not on harness (where it is still downloaded, but for nothing since it is also in the zip file). A followup bug would be nice!

> plugins- we really don't use these anymore- we should file a bug to cleanup the talos.json and ensure that plugins are not used at all.  These were supposed to be flash plugins, but they are years old- sort of invalid at this point.

agreed for that.

> we have python_webserver=False here, but adding --develop forces it to true, can we simplify that a bit?  Maybe just wait until we get the python webserver used in automation?

Yes, we can probably (and that would be nice, I think --develop should just go away). We can tackle this is a follow up.
Thanks :jmaher for the review! I fixed the issue, I think other comments should be addressed in other bugs. So once you are sure that the tests run are the same - it should, since the definition is still read from the talos.json file - if you're happy with that patch, please land the patch or ping me so I can do it. :)
lets try the checkin-needed, otherwise I would appreciate if you could land it tomorrow.  I will be in a lot of meetings tomorrow without a chance to really be responsible for landing.
Keywords: checkin-needed
See Also: → 1207071
See Also: → 1207074
https://hg.mozilla.org/mozilla-central/rev/545b59854de3
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in before you can comment on or make changes to this bug.