573462 - [mozmill] Mozmill tests fail due to ServerURL changes in application.ini

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Since a couple of days our Mozmill test which checks the server url fails because the target url has gotten parameters now. That means we will have to update our test.

Failure:
/firefox/testInstallation/testBreakpadInstalled.js 	testBreakpadInstalled 		Controller.assertJS("subject.reporter.serverURL.spec == subject.stateServerURL")

We should push the fix to all branches. Geo, can you please work on it?

Comment 1

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Attachment: Changed ServerURL comparison to be a regex match instead, to accomodate parameters

Here's a first shot at a fix.  The various parameter values are very install/version/build specific, so unless they're independently gettable somewhere we can't really do a direct == anymore.  

So, I put in a regex check that the URL starts with the unchanging bits and has parameters.

This should be applicable to all the branches--do I have to do anything special to let you do that, or is the one patch enough?

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Before I review the patch I would like to know the reason of the change.

Ted, can you point us to the implementation bug? Would be great to get some background information. Do we already have tests for the parameters or should we do that in Mozmill?

Comment 3

[(not currently active) Ted Mielczarek]

Sorry, this was bug 521349. It's just to provide more information for the collector logs for easy data mining.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #3)
> Sorry, this was bug 521349. It's just to provide more information for the
> collector logs for easy data mining.

Thanks. Does it mean it's irrelevant for our Mozmill test and we should simply check that the server url is correct?

Comment 5

[(not currently active) Ted Mielczarek]

I don't know that there's much reason to test this value, and we're not actually processing the parameters, no.

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 453459 [details] [diff] [review]
Changed ServerURL comparison to be a regex match instead, to accomodate parameters

>-                "ServerURL" : "https://crash-reports.mozilla.com/submit"
>+                "ServerURLPattern" : /^https:\/\/crash-reports\.mozilla\.com\/submit\?.*/

Geo, can you place make the question mark optional? I would like to land this fix for all branches so it would also have to handle a non-parametrized URL. That way we can also make sure that the test on both branches will not break once the serverURL change will land for Firefox 3.6 and Firefox 3.5.

>-  controller.assertJS("subject.reporter.serverURL.spec == subject.stateServerURL",
>-                      {reporter: crashReporter, stateServerURL: states['ServerURL']});
>+  controller.assertJS("subject.reporter.serverURL.spec.search(subject.stateServerURLPattern) != -1",
>+                      {reporter: crashReporter, 
>+                       stateServerURLPattern: states['ServerURLPattern']});

Can you please move the comparison from inside the eval string to the object parameter and pass in only one object member?

r=me with those changes.

Comment 7

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Attachment: As above, plus changed ServerURLPattern to not check for ? (old branches) and restructured comparison

I tried restructuring the comparison as you suggested, but it just moved all the complexity down a line and the code wasn't really improved.  

See if how I did it here is clearer.  If not, I'll do a quick change to send the boolean in instead.

I did change the pattern so it no longer cares if ? is there, and tried it on older builds with no problems.  Great catch!

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 453841 [details] [diff] [review]
As above, plus changed ServerURLPattern to not check for ? (old branches) and restructured comparison

>-  controller.assertJS("subject.reporter.serverURL.spec == subject.stateServerURL",
>-                      {reporter: crashReporter, stateServerURL: states['ServerURL']});
>+  controller.assertJS("subject.pos != -1",
>+                      {pos: crashReporter.serverURL.spec.search(states['ServerURLPattern'])});

We definitely need a good failure output. Having "subject.pos != -1" in case of a failure isn't really helpful. What about:

>+  var index = crashReporter.serverURL.spec.search(states['ServerURLPattern']);
>+  controller.assertJS("subject.expectedCrashReportURL == true",
>+                      {expectedCrashReportURL: index != -1});

That way we can immediately see what has been failed. Geo, if you can update the patch quickly it would be great. With the update r=me.

Comment 9

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Attachment: As above, restructured comparison to make test output clearer

Took Henrik's suggestion verbatim.  I think it's probably the cleanest way to structure that block.

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 453857 [details] [diff] [review]
As above, restructured comparison to make test output clearer

># HG changeset patch
># User Geo Mealer <[e-mail]>
># Date 1277414709 25200
># Node ID ff5598367974f14842427b4cf10e6aba6f65db50
># Parent  43a3c1310d3143630476703e68450a36fd7cceb6
>Changed ServerURL comparison to be a regex match instead, to accomodate parameters

Nit: Next time please also add the bug number and the reviewer at the end. That will help me a lot. I will update it this time. Thanks.

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Checked in on default and 1.9.2:
http://hg.mozilla.org/qa/mozmill-tests/rev/24c6f02187ed
http://hg.mozilla.org/qa/mozmill-tests/rev/1dd54e6d9eb8

I haven't checked it into 1.9.1 because it requires a backport. I think for now we are ok. But once the patch on bug 521349 gets approved for 1.9.1 we will also have to come up with a backport here. Until that date lets focus on other failed tests.

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Mass move of Mozmill Test related project bugs to newly created components. You can filter out those emails by using "Mozmill-Tests-to-MozillaQA" as criteria.