899171 - Use mozprofile in the reftest harness

Status: RESOLVED FIXED

(Testing :: Reftest, defect)

Comment 1

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: Use mozprofile in the reftest harness

Comment 2

[Mihnea Dobrescu-Balaur (:mihneadb)]

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?

Comment 3

[Mihnea Dobrescu-Balaur (:mihneadb)]

Whoops.. I really got used to starting xpcshell tests!
https://tbpl.mozilla.org/?tree=Try&rev=975b93e06cb2

Comment 4

[Andrew Halberstadt [:ahal]]

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.

Comment 5

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: Use mozprofile in the reftest harness

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

Comment 6

[Mihnea Dobrescu-Balaur (:mihneadb)]

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.

Comment 7

[Andrew Halberstadt [:ahal]]

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

Comment 8

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: Use mozprofile in the reftest harness

Added the mobile changes.

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

Comment 9

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: 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.

Comment 10

[Mihnea Dobrescu-Balaur (:mihneadb)]

https://tbpl.mozilla.org/?tree=Try&rev=9e46dde6ed71

Comment 11

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: 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

Comment 12

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: Use mozprofile in the reftest harness

mobile createReftestProfile was supposed to return profile as well.

Comment 13

[Mihnea Dobrescu-Balaur (:mihneadb)]

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

Comment 14

[Mihnea Dobrescu-Balaur (:mihneadb)]

Scheduled a new try run to see if the B2G stuff works now.

https://tbpl.mozilla.org/?tree=Try&rev=2ba9a0c59cee

Comment 15

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: Use mozprofile in the reftest harness

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

Comment 16

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: Use mozprofile in the reftest harness

Fixed the syntax error.

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

Comment 17

[Mihnea Dobrescu-Balaur (:mihneadb)]

Comment on attachment 786645 [details] [diff] [review]
Use mozprofile in the reftest harness

Ok, that seems to have fixed it.

Comment 18

[Andrew Halberstadt [:ahal]]

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.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3354e7e52613

Comment 20

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/10553de130e1 for making Android reftests and jsreftests, and b2g crashtests, rather angry.

Comment 21

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: 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.

Comment 22

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: 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

Comment 23

[Mihnea Dobrescu-Balaur (:mihneadb)]

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

Comment 24

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: Use mozprofile in the reftest harness

Comment 25

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: Use mozprofile in the reftest harness

Ok, hopefully this fixes it.

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

Comment 26

[Mihnea Dobrescu-Balaur (:mihneadb)]

Comment on attachment 792437 [details] [diff] [review]
Use mozprofile in the reftest harness

This seems to work! r?

Comment 27

[Andrew Halberstadt [:ahal]]

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.

Comment 28

[Mihnea Dobrescu-Balaur (:mihneadb)]

Will do, thanks!

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/de107e9ab150

Comment 30

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/de107e9ab150