Closed
Bug 899171
Opened 11 years ago
Closed 11 years ago
Use mozprofile in the reftest harness
Categories
(Testing :: Reftest, defect)
Testing
Reftest
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla26
People
(Reporter: mihneadb, Assigned: mihneadb)
References
Details
Attachments
(1 file, 11 obsolete files)
20.92 KB,
patch
|
ahal
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → mihneadb
Assignee | ||
Comment 1•11 years ago
|
||
Assignee | ||
Comment 2•11 years ago
|
||
Comment on attachment 782866 [details] [diff] [review] Use mozprofile in the reftest harness https://tbpl.mozilla.org/?tree=Try&rev=3ee624849a2c Does this make sense?
Attachment #782866 -
Flags: feedback?(ahalberstadt)
Assignee | ||
Comment 3•11 years ago
|
||
Whoops.. I really got used to starting xpcshell tests! https://tbpl.mozilla.org/?tree=Try&rev=975b93e06cb2
Comment 4•11 years ago
|
||
Comment on attachment 782866 [details] [diff] [review] Use mozprofile in the reftest harness Review of attachment 782866 [details] [diff] [review]: ----------------------------------------------------------------- This looks like a good start. You'll probably need to do some stuff to get b2g and android working as well. The following comment isn't specific to this patch, just a comment about the whole refactor in general. Don't worry about "preserving the way things were done before". Don't be afraid to tear the file apart if you think things could be done better in a different way. Though getting too carried away will make it more difficult to land, so a larger amount of smaller focused patches isn't a bad thing (I'll leave it to your discretion). ::: layout/tools/reftest/runreftest.py @@ +50,5 @@ > 'manifest' is the path to the reftest.list file we want to test with. This is used in > the remote subclass in remotereftest.py so we can write it to a preference for the > bootstrap extension. > """ > + profileDir = profile.profile Seems redundant to use profileDir when the value is already stored in profile.profile. @@ +128,2 @@ > self.copyExtraFilesToProfile(options, profileDir) > + self.createReftestProfile(options, profile, reftestlist) This is kind of an awkward flow. I would just create the profile inside createReftestProfile and call copyExtraFilesToProfile afterwards. That way you can just pass all the various pieces directly into the Profile constructor instead of having to call things like set_preferences and install_from_path. @@ +130,1 @@ > self.installExtensionsToProfile(options, profileDir) installExtensionsToProfile should be replaced by passing 'addons' into the profile constructor.
Attachment #782866 -
Flags: feedback?(ahalberstadt) → feedback+
Assignee | ||
Comment 5•11 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=f00e17f0851d
Assignee | ||
Updated•11 years ago
|
Attachment #782866 -
Attachment is obsolete: true
Assignee | ||
Comment 6•11 years ago
|
||
Comment on attachment 784034 [details] [diff] [review] Use mozprofile in the reftest harness Ok, this seems to work (see try run). Let me know if I should do something in a different way. If not, I'll go ahead and update the mobile harnesses.
Attachment #784034 -
Flags: review?(ahalberstadt)
Comment 7•11 years ago
|
||
Comment on attachment 784034 [details] [diff] [review] Use mozprofile in the reftest harness Review of attachment 784034 [details] [diff] [review]: ----------------------------------------------------------------- Looks good! Though, yes this will break mobile. ::: layout/tools/reftest/runreftest.py @@ +134,5 @@ > if cmdlineArgs == None: > cmdlineArgs = ['-reftest', reftestlist] > + profile = self.createReftestProfile(options, reftestlist) > + profileDir = profile.profile # name makes more sense > + self.copyExtraFilesToProfile(options, profile.profile) if you're going to create a profileDir variable, at least use it :p
Attachment #784034 -
Flags: review?(ahalberstadt) → review+
Assignee | ||
Comment 8•11 years ago
|
||
Added the mobile changes. https://tbpl.mozilla.org/?tree=Try&rev=f28b98c44743
Assignee | ||
Updated•11 years ago
|
Attachment #784034 -
Attachment is obsolete: true
Assignee | ||
Comment 9•11 years ago
|
||
Fixed a NameError when assigning profileDir in the try scope would not happen. Stopped the try run, triggering a new one.
Assignee | ||
Updated•11 years ago
|
Attachment #784570 -
Attachment is obsolete: true
Assignee | ||
Comment 10•11 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=9e46dde6ed71
Assignee | ||
Comment 11•11 years ago
|
||
Forgot to convert false to False (js to python literal). Cancelled the try run, started a new one: https://tbpl.mozilla.org/?tree=Try&rev=43542ea06cd1
Assignee | ||
Updated•11 years ago
|
Attachment #784650 -
Attachment is obsolete: true
Assignee | ||
Comment 12•11 years ago
|
||
mobile createReftestProfile was supposed to return profile as well.
Assignee | ||
Updated•11 years ago
|
Attachment #784683 -
Attachment is obsolete: true
Assignee | ||
Comment 13•11 years ago
|
||
B2G mozprofile bits seem broken (check errors here - https://tbpl.mozilla.org/?tree=Try&rev=17e35e4faf86 )
Assignee | ||
Updated•11 years ago
|
Attachment #784735 -
Flags: review?(ahalberstadt)
Assignee | ||
Comment 14•11 years ago
|
||
Scheduled a new try run to see if the B2G stuff works now. https://tbpl.mozilla.org/?tree=Try&rev=2ba9a0c59cee
Assignee | ||
Comment 15•11 years ago
|
||
Found some b2g stuff that needed addressing. New try run: https://tbpl.mozilla.org/?tree=Try&rev=5423ed932cfe
Assignee | ||
Updated•11 years ago
|
Attachment #784735 -
Attachment is obsolete: true
Attachment #784735 -
Flags: review?(ahalberstadt)
Assignee | ||
Comment 16•11 years ago
|
||
Fixed the syntax error. https://tbpl.mozilla.org/?tree=Try&rev=b70042dbfcae
Assignee | ||
Updated•11 years ago
|
Attachment #786393 -
Attachment is obsolete: true
Assignee | ||
Comment 17•11 years ago
|
||
Comment on attachment 786645 [details] [diff] [review] Use mozprofile in the reftest harness Ok, that seems to have fixed it.
Attachment #786645 -
Flags: review?(ahalberstadt)
Comment 18•11 years ago
|
||
Comment on attachment 786645 [details] [diff] [review] Use mozprofile in the reftest harness Review of attachment 786645 [details] [diff] [review]: ----------------------------------------------------------------- Awesome, thanks! The prefs should be pulled out of the harness and into testing/profiles, but we can do that in a separate bug.
Attachment #786645 -
Flags: review?(ahalberstadt) → review+
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
Comment 19•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/3354e7e52613
Keywords: checkin-needed
Comment 20•11 years ago
|
||
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/10553de130e1 for making Android reftests and jsreftests, and b2g crashtests, rather angry.
Assignee | ||
Comment 21•11 years ago
|
||
Fixed the crashtest specialpowers thing for B2G. Try run here: https://tbpl.mozilla.org/?tree=Try&rev=b86558c6b38b Still not done, waiting to see what's up with Android. The error on 4.0 seems unrelated to this patch. We also don't seem to run 4.0 on inbound/central.
Assignee | ||
Updated•11 years ago
|
Attachment #786645 -
Attachment is obsolete: true
Assignee | ||
Comment 22•11 years ago
|
||
Think I found the bug for the android jsreftest failures - the copyextrafiles[..] method was not being called anymore. Try run: https://tbpl.mozilla.org/?tree=Try&rev=6143451a39f6
Assignee | ||
Updated•11 years ago
|
Attachment #790929 -
Attachment is obsolete: true
Assignee | ||
Comment 23•11 years ago
|
||
So JSReftests are still orange, but you can see in this[1] log that they were running. I'll dig some more, but if you have any suggestions, I'm listening! [1] https://tbpl.mozilla.org/php/getParsedLog.php?id=26674514&tree=Try&full=1
Assignee | ||
Comment 24•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Attachment #791558 -
Attachment is obsolete: true
Assignee | ||
Comment 25•11 years ago
|
||
Ok, hopefully this fixes it. https://tbpl.mozilla.org/?tree=Try&rev=6d215fff563d
Assignee | ||
Updated•11 years ago
|
Attachment #792321 -
Attachment is obsolete: true
Assignee | ||
Comment 26•11 years ago
|
||
Comment on attachment 792437 [details] [diff] [review] Use mozprofile in the reftest harness This seems to work! r?
Attachment #792437 -
Flags: review?(ahalberstadt)
Comment 27•11 years ago
|
||
Comment on attachment 792437 [details] [diff] [review] Use mozprofile in the reftest harness Review of attachment 792437 [details] [diff] [review]: ----------------------------------------------------------------- This looks good. For future reference though, if there's a big patch that already got r+'ed could you upload an interdiff? That way it's easier to see the changes that need to be reviewed.
Attachment #792437 -
Flags: review?(ahalberstadt) → review+
Comment 29•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/de107e9ab150
Flags: in-testsuite-
Keywords: checkin-needed
Comment 30•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/de107e9ab150
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
You need to log in
before you can comment on or make changes to this bug.
Description
•