Closed
Bug 1153992
Opened 10 years ago
Closed 9 years ago
Autophone - allow test manifest to include multiple sections per test script
Categories
(Testing Graveyard :: Autophone, enhancement)
Testing Graveyard
Autophone
Tracking
(firefox40 affected)
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
firefox40 | --- | affected |
People
(Reporter: bc, Assigned: bc)
References
Details
Attachments
(2 files)
Currently we have a restriction in test manifests where there is a one-one mapping between the section specifying the test script and the config option which lists the config files to be used.
For example:
[runtestsremote.py]
config = ../configs/mochitests_dom_browser_element_settings.ini ../configs/mochitests_dom_media_settings.ini ../configs/mochitests_skia_settings.ini ../configs/mochitests_toolkit_widgets_settings.ini ../configs/mochitests_settings.ini
nexus-one-3 = fx-team
samsung-gs3-3 = mozilla-inbound try
Recently we added the mochitests-media tests and wish to not run the older mochitests by default but would like to be able to run the older mochitests on try. For example, I want to be able to do something like:
[runtestsremote.py]
config = ../configs/mochitests_dom_browser_element_settings.ini ../configs/mochitests_dom_media_settings.ini ../configs/mochitests_skia_settings.ini ../configs/mochitests_toolkit_widgets_settings.ini
samsung-gs3-3 = try
[runtestsremote.py]
config = ../configs/mochitests_media.ini
nexus-one-3 = fx-team try
This won't work since we need the section names to be unique.
In order to make the section unique, I think it would suffice to add a unique id to the section as in:
[runtestsremote.py 1]
config = ../configs/mochitests_dom_browser_element_settings.ini ../configs/mochitests_dom_media_settings.ini ../configs/mochitests_skia_settings.ini ../configs/mochitests_toolkit_widgets_settings.ini ../configs/mochitests_settings.ini
samsung-gs3-3 = try
[runtestsremote.py 2]
config = ../configs/mochitests_media.ini
nexus-one-3 = fx-team try
In autophone.read_tests, we could hack this in by adjusting the t['name'] property
for t in tests_info:
t['name'] = t['name'].split()[0]
I've hacked a quick patch together locally which shows this will work.
Assignee | ||
Comment 1•10 years ago
|
||
mcote: I'd appreciate your opinion before I add a pr.
Flags: needinfo?(mcote)
Comment 2•10 years ago
|
||
Just want to make sure I understand: what you really want is to be able to specify different phone-config associations for the same test?
Flags: needinfo?(mcote)
Assignee | ||
Comment 3•10 years ago
|
||
Yes.
Comment 4•10 years ago
|
||
Hm. I'm not crazy about the idea of using arbitrary, semi-magic numbers... but I can't think of a better way without switching to a proper hierarchical config format like YAML. Maybe just using a descriptive string instead of a number, since (I imagine) everything after the first space will be ignored anyway.
Assignee | ||
Comment 5•10 years ago
|
||
Yeah, I wasn't married to the number. It was just for illustrative purposes. Like you said, the remainder of the section name is irrelevant to the test.
Assignee | ||
Comment 6•10 years ago
|
||
Asking gbrown for review as this will be a good time to get familiar with some of the autophone internals. mcote, If you want to give feedback that is fine but not required.
Being able to split the test manifest's section names was the easy part.
It uncovered an outstanding issue that it was not possible to identify a chunked test properly for cancelling or retriggering. It also highlighted the need to be able to identify tests by their names as well as their config file and their chunk.
Adding the config_file and chunk to the tests table necessitated changes to the database as well. I took this opportunity to do the refactoring mcote suggested and also changed the duplicate checking to check for individual duplicate tests rather than the set of tests as duplicating an existing set of tests. This was necessary since we had already begun deleting tests as they completed which made the old check for a duplicate set of tests obsolete.
The jobs.py file is almost completely rewritten and you should probably review the file rather than just the patch.
I also consolidated the selection of runnable tests using a new classmethod PhoneTest.match and used that in on_build and on_jobaction and trigger_jobs to select the actual test objects before submitting them to AutoPhone.new_job. This eliminated the need for duplicate api and abi and test device repo checking. This involved creating a dict containing all of the PhoneTest instances created in a process.
The config_file and chunk were added to the privatebuild treeherder artifact and the config_file and build url were added to the job_details artifact to help identify the actual test in the treeherder job details panel.
I changed PhoneWorker.cancel_job and PhoneWorkerSubProcess.cancel_job to cancel_test to make it better reflect what it really does.
Additional nits:
removed the urlparse stuff from autophone.py since it was unused.
tweaked variables for test names to be test_name(s) rather than tests to make it clear what the variable was for a test name and not a test object.
tweaked the buildname to use the test name instead of the job symbol as it better identified the chunk.
You can see an example run
https://treeherder.allizom.org/#/jobs?repo=mozilla-inbound&revision=03da3b6d21b9&filter-searchStr=autophone
where I split the tests into local blank, remote blank, local twitter, remote twitter, local nytimes and remote nytimes using a test manifest. See attachment.
Attachment #8596124 -
Flags: review?(gbrown)
Attachment #8596124 -
Flags: feedback?(mcote)
Assignee | ||
Comment 7•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Comment 8•10 years ago
|
||
I don't think there is cause to change this now, but I would have preferred to see the script separated entirely from the section name.
Instead of:
[s1s2test.py blank]
config = ../configs/s1s2_blank_local.ini ../configs/s1s2_blank_remote.ini
nexus-one-1 = mozilla-inbound
[s1s2test.py twitter]
config = ../configs/s1s2_twitter_local.ini ../configs/s1s2_twitter_remote.ini
nexus-one-2 = mozilla-inbound
I would prefer:
[s1s2-blank]
script = s1s2test.py
config = ../configs/s1s2_blank_local.ini ../configs/s1s2_blank_remote.ini
nexus-one-1 = mozilla-inbound
[s1s2-twitter]
script = s1s2test.py
config = ../configs/s1s2_twitter_local.ini ../configs/s1s2_twitter_remote.ini
nexus-one-2 = mozilla-inbound
Comment 9•10 years ago
|
||
Comment on attachment 8596124 [details]
pr 24
Sorry for the long wait -- it was a busy week! In the end, I didn't find any major concerns, just jotted down a few ideas for possible changes, at your discretion.
Attachment #8596124 -
Flags: review?(gbrown) → review+
Assignee | ||
Comment 10•10 years ago
|
||
Comment on attachment 8596124 [details]
pr 24
cursor.fetch... will raise a ProgrammingError if the cursor is closed but from what I can tell it won't raise anything else.
cursor.close also appears to raise nothing. multiple closes do nothing.
I did change cancel_test to remove an early return that was actually less better than the conditional.
I've added a commit to the branch which you can review quickly. I'll squash it into the previous commit before merging/pushing.
Attachment #8596124 -
Flags: feedback?(mcote) → review?(gbrown)
Updated•9 years ago
|
Attachment #8596124 -
Flags: review?(gbrown) → review+
Assignee | ||
Comment 11•9 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Updated•3 years ago
|
Product: Testing → Testing Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•