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)

enhancement
Not set
normal

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.
mcote: I'd appreciate your opinion before I add a pr.
Flags: needinfo?(mcote)
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)
Yes.
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.
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.
Attached file pr 24 (deleted) —
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)
Attached file example test manifest (deleted) —
Blocks: 1151882, 1150111
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 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+
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)
Attachment #8596124 - Flags: review?(gbrown) → review+
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Blocks: 1160155
Blocks: 1155876
Depends on: 1162225
No longer blocks: 1160155
Depends on: 1162514
Blocks: 1163129
Depends on: 1163340
Product: Testing → Testing Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: