Use mozprofile in the reftest harness

RESOLVED FIXED in mozilla26

Status

Testing
Reftest
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: mihneadb, Assigned: mihneadb)

Tracking

unspecified
mozilla26
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 11 obsolete attachments)

20.92 KB, patch
ahal
: review+
Details | Diff | Splinter Review
Comment hidden (empty)
(Assignee)

Updated

4 years ago
Assignee: nobody → mihneadb
(Assignee)

Comment 1

4 years ago
Created attachment 782866 [details] [diff] [review]
Use mozprofile in the reftest harness
(Assignee)

Comment 2

4 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

4 years ago
Whoops.. I really got used to starting xpcshell tests!
https://tbpl.mozilla.org/?tree=Try&rev=975b93e06cb2
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

4 years ago
Created attachment 784034 [details] [diff] [review]
Use mozprofile in the reftest harness

https://tbpl.mozilla.org/?tree=Try&rev=f00e17f0851d
(Assignee)

Updated

4 years ago
Attachment #782866 - Attachment is obsolete: true
(Assignee)

Comment 6

4 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 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

4 years ago
Created attachment 784570 [details] [diff] [review]
Use mozprofile in the reftest harness

Added the mobile changes.

https://tbpl.mozilla.org/?tree=Try&rev=f28b98c44743
(Assignee)

Updated

4 years ago
Attachment #784034 - Attachment is obsolete: true
(Assignee)

Comment 9

4 years ago
Created attachment 784650 [details] [diff] [review]
Use mozprofile in the reftest harness

Fixed a NameError when assigning profileDir in the try scope would not happen.

Stopped the try run, triggering a new one.
(Assignee)

Updated

4 years ago
Attachment #784570 - Attachment is obsolete: true
(Assignee)

Comment 10

4 years ago
https://tbpl.mozilla.org/?tree=Try&rev=9e46dde6ed71
(Assignee)

Comment 11

4 years ago
Created attachment 784683 [details] [diff] [review]
Use mozprofile in the reftest harness

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

4 years ago
Attachment #784650 - Attachment is obsolete: true
(Assignee)

Comment 12

4 years ago
Created attachment 784735 [details] [diff] [review]
Use mozprofile in the reftest harness

mobile createReftestProfile was supposed to return profile as well.
(Assignee)

Updated

4 years ago
Attachment #784683 - Attachment is obsolete: true
(Assignee)

Comment 13

4 years ago
B2G mozprofile bits seem broken (check errors here - https://tbpl.mozilla.org/?tree=Try&rev=17e35e4faf86 )
(Assignee)

Updated

4 years ago
Attachment #784735 - Flags: review?(ahalberstadt)
(Assignee)

Comment 14

4 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

4 years ago
Created attachment 786393 [details] [diff] [review]
Use mozprofile in the reftest harness

Found some b2g stuff that needed addressing. New try run:
https://tbpl.mozilla.org/?tree=Try&rev=5423ed932cfe
(Assignee)

Updated

4 years ago
Attachment #784735 - Attachment is obsolete: true
Attachment #784735 - Flags: review?(ahalberstadt)
(Assignee)

Comment 16

4 years ago
Created attachment 786645 [details] [diff] [review]
Use mozprofile in the reftest harness

Fixed the syntax error.

https://tbpl.mozilla.org/?tree=Try&rev=b70042dbfcae
(Assignee)

Updated

4 years ago
Attachment #786393 - Attachment is obsolete: true
(Assignee)

Comment 17

4 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 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

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/3354e7e52613
Keywords: checkin-needed
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

4 years ago
Created attachment 790929 [details] [diff] [review]
Use mozprofile in the reftest harness

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

4 years ago
Attachment #786645 - Attachment is obsolete: true
(Assignee)

Comment 22

4 years ago
Created attachment 791558 [details] [diff] [review]
Use mozprofile in the reftest harness

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

4 years ago
Attachment #790929 - Attachment is obsolete: true
(Assignee)

Comment 23

4 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

4 years ago
Created attachment 792321 [details] [diff] [review]
Use mozprofile in the reftest harness
(Assignee)

Updated

4 years ago
Attachment #791558 - Attachment is obsolete: true
(Assignee)

Comment 25

4 years ago
Created attachment 792437 [details] [diff] [review]
Use mozprofile in the reftest harness

Ok, hopefully this fixes it.

https://tbpl.mozilla.org/?tree=Try&rev=6d215fff563d
(Assignee)

Updated

4 years ago
Attachment #792321 - Attachment is obsolete: true
(Assignee)

Comment 26

4 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 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+
(Assignee)

Comment 28

4 years ago
Will do, thanks!
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/de107e9ab150
Flags: in-testsuite-
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/de107e9ab150
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26

Updated

4 years ago
Depends on: 917576
You need to log in before you can comment on or make changes to this bug.