Last Comment Bug 760374 - testBlueLarry fails with Disconnect error
: testBlueLarry fails with Disconnect error
Status: RESOLVED FIXED
[mozmill-test-failure]
:
Product: Mozilla QA
Classification: Other
Component: Mozmill Tests (show other bugs)
: unspecified
: All All
: -- critical (vote)
: ---
Assigned To: Maniac Vlad Florin (:vladmaniac)
:
:
Mentors:
http://mozmill-ci.blargon7.com/#/func...
Depends on: 661121
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-31 23:30 PDT by Anthony Hughes (:ashughes) [GFX][QA][Mentor]
Modified: 2012-08-13 04:44 PDT (History)
5 users (show)
vlad.mozbugs: in‑litmus+
See Also:
QA Whiteboard:
Iteration: ---
Points: ---
fixed
verified
fixed
fixed
verified


Attachments
patch v 1.0 (2.85 KB, patch)
2012-06-26 05:03 PDT, Maniac Vlad Florin (:vladmaniac)
hskupin: review-
Details | Diff | Splinter Review
patch v2.0 (3.59 KB, patch)
2012-06-27 01:45 PDT, Maniac Vlad Florin (:vladmaniac)
hskupin: review-
Details | Diff | Splinter Review
patch v3.0 (3.47 KB, patch)
2012-06-27 07:47 PDT, Maniac Vlad Florin (:vladmaniac)
hskupin: review-
Details | Diff | Splinter Review
patch v4.0 (3.59 KB, patch)
2012-06-27 09:39 PDT, Maniac Vlad Florin (:vladmaniac)
hskupin: review-
Details | Diff | Splinter Review
patch v5.0 (3.83 KB, patch)
2012-07-05 02:24 PDT, Maniac Vlad Florin (:vladmaniac)
hskupin: review-
Details | Diff | Splinter Review
patch v5.1 (3.71 KB, patch)
2012-07-05 05:30 PDT, Maniac Vlad Florin (:vladmaniac)
hskupin: review-
Details | Diff | Splinter Review
patch v5.2 (3.73 KB, patch)
2012-07-09 23:56 PDT, Maniac Vlad Florin (:vladmaniac)
hskupin: review-
Details | Diff | Splinter Review
patch v5.3 (3.86 KB, patch)
2012-07-12 07:58 PDT, Maniac Vlad Florin (:vladmaniac)
hskupin: review-
Details | Diff | Splinter Review
patch v5.4 (3.88 KB, patch)
2012-07-16 00:27 PDT, Maniac Vlad Florin (:vladmaniac)
hskupin: review+
Details | Diff | Splinter Review
[mozilla-esr10] patch v1.0 (4.79 KB, patch)
2012-07-16 23:58 PDT, Maniac Vlad Florin (:vladmaniac)
hskupin: review+
Details | Diff | Splinter Review

Description Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-05-31 23:30:19 PDT
Running the ondemand scripts today for the 10.0.5esr builds, testBlueLarry.js reported a Disconnect error twice. Once on Windows 7 and again on Windows XP:

http://mozmill-ondemand.blargon7.com/#/functional/report/fdec829b93b19c73985be1d38840c448
http://mozmill-ondemand.blargon7.com/#/functional/report/fdec829b93b19c73985be1d38840e1d3
Comment 1 Maniac Vlad Florin (:vladmaniac) 2012-06-01 00:03:41 PDT
This is also for mac 
http://mozmill-ondemand.blargon7.com/#/functional/report/fdec829b93b19c73985be1d38841629c
Comment 2 Henrik Skupin (:whimboo) 2012-06-01 00:04:09 PDT
Vlad will take care of it. Just checked quickly and it started to happen on May 25th. Could be a Firefox regression?
Comment 3 Maniac Vlad Florin (:vladmaniac) 2012-06-01 01:23:52 PDT
I cannot trigger this disconnect locally 

http://mozmill-crowd.blargon7.com/#/functional/reports?branch=All&platform=All&from=2012-06-01&to=2012-06-01 

are we sure this is not system dependent? If time permits, I would suggest someone to start a testrun on the exact machine we are seeing this. 

I the meantime, I will do a hg bisect to see if we have some changes in the Security UI
Comment 4 Maniac Vlad Florin (:vladmaniac) 2012-06-01 02:27:09 PDT
Don't see any changes related to Security UI in 
http://hg.mozilla.org/releases/mozilla-esr10/pushloghtml?fromchangeset=282ed6ece1f6&tochangeset=5713c92407dd 

So is it remote environment dependent? My mac mini is still running tests and reporting to mozmill-crowd. Hope we catch something
Comment 5 Maniac Vlad Florin (:vladmaniac) 2012-06-05 05:55:27 PDT
Have lots of local reports for this but no error. 
Also I see nothing of this on mozmill-release in the last 5 days 
This occurred only on Mac OS X 10.6.8 four times in one month 

http://mozmill-release.blargon7.com/#/functional/failure?branch=10.0&platform=Mac&from=2012-05-06&to=2012-06-05&test=%2FtestSecurity%2FtestBlueLarry.js&func=testLarryBlue
Comment 6 Henrik Skupin (:whimboo) 2012-06-07 08:45:03 PDT
Given that it happens that rarely lets close this bug for now. Once it manifests again we should check the failure on the same day. Could be that there were some issues with external sites or whatever.
Comment 7 Henrik Skupin (:whimboo) 2012-06-12 23:09:14 PDT
This happened yesterday with an ESR10 build:
http://mozmill-ci.blargon7.com/#/functional/report/ee284dfa9d664754a65676abbc1300df

Could someone please try if it is reproducible?
Comment 8 Remus Pop (:RemusPop) 2012-06-13 05:39:29 PDT
Managed to reproduce it with low memory in a Mac. Before starting Firefox, ~150 MB were available in RAM. It failed 2 times out of 30. Disk space is ~25 MB.
I cannot reproduce it 100% and it does not leave a Firefox open.
I'll try to tun the folder alone.
Comment 9 Remus Pop (:RemusPop) 2012-06-13 05:40:50 PDT
Oh, sorry, this were the results for the beta candidate:
http://mozmill-crowd.blargon7.com/#/functional/report/e67171ea696231bb192f565615041b76
Comment 10 Henrik Skupin (:whimboo) 2012-06-25 03:35:13 PDT
Vlad, please transform this test to make use of mozqa.com. There we have a DV cert available. Lets hope that it will make a difference.
Comment 11 Henrik Skupin (:whimboo) 2012-06-25 10:45:44 PDT
This is critical. So please figure out who can do this change early tomorrow. thanks!
Comment 12 Maniac Vlad Florin (:vladmaniac) 2012-06-26 05:03:58 PDT
Created attachment 636660 [details] [diff] [review]
patch v 1.0

Modified test to use one of our ssl - dv cert pages from mozqa.com
Comment 13 Henrik Skupin (:whimboo) 2012-06-26 05:45:37 PDT
Comment on attachment 636660 [details] [diff] [review]
patch v 1.0

>+const LOCAL_TEST_PAGE = "https://ssl-dv.mozqa.com/data/" + 
>+                        "firefox/layout/mozilla_community.html";

This is not a local test page. Beside that just use the plain domain.

>-  controller.assertValue(webIDDomainLabel, cert.commonName.replace("*", "mail"));
>+  controller.assertValue(webIDDomainLabel, cert.commonName.replace("*", "ssl-dv"));
[..]
>   controller.assertValue(webIDOwnerLabel, securityOwner);

While you are on it please change this to an expect call. Whereby you don't have to do any replacement. This cert is exclusively for this sub domain.
Comment 14 Maniac Vlad Florin (:vladmaniac) 2012-06-27 01:45:09 PDT
> 
> While you are on it please change this to an expect call. Whereby you don't
> have to do any replacement. This cert is exclusively for this sub domain.

I still have to do the replacement because webIDDOmainLabel will be the full domain name, and cert.commonName will be '*.mozqa.com'. The expect.equal does not change that. If I understood what you meant there by 'replacement'
Comment 15 Maniac Vlad Florin (:vladmaniac) 2012-06-27 01:45:55 PDT
Created attachment 637033 [details] [diff] [review]
patch v2.0

r=hskupin this time because its the second iteration
Comment 16 Henrik Skupin (:whimboo) 2012-06-27 02:39:05 PDT
Comment on attachment 637033 [details] [diff] [review]
patch v2.0

>+const TEST_PAGE = "https://ssl-dv.mozqa.com";

In this case I still see no reason why to introduce a global constant. Just leave it as is in the test.

>-  controller.assertValue(webIDDomainLabel, cert.commonName.replace("*", "mail"));
>+  expect.equal(webIDDomainLabel.getNode().value, cert.commonName.replace("*", "ssl-dv"), 
>+               "Web Site label equals '" + cert.commonName + "'");

Please read my review comment correctly. There is no need for a replace.

>+  expect.equal(webIDOwnerLabel.getNode().value, securityOwner, 
>+               "Owner label equals '" + securityOwner + "'");

Putting the variable you already have as 2nd argument in the message doesn't make sense. It's just duplicated. Instead say "Expected owner label found" or similar.

>-  controller.assertValue(webIDVerifierLabel, cert.issuerOrganization);
>+  expect.equal(webIDVerifierLabel.getNode().value, cert.issuerOrganization, 
>+               "Verifier label equals '" + cert.issuerOrganization + "'");

Same here.
Comment 17 Henrik Skupin (:whimboo) 2012-06-27 02:41:17 PDT
(In reply to Maniac Vlad Florin (:vladmaniac) from comment #14)
> I still have to do the replacement because webIDDOmainLabel will be the full
> domain name, and cert.commonName will be '*.mozqa.com'. The expect.equal
> does not change that. If I understood what you meant there by 'replacement'

User expect.match() instead of expect.equal(). But can you show me the logging output for that?
Comment 18 Maniac Vlad Florin (:vladmaniac) 2012-06-27 02:45:12 PDT
(In reply to Henrik Skupin (:whimboo) from comment #17)
> (In reply to Maniac Vlad Florin (:vladmaniac) from comment #14)
> > I still have to do the replacement because webIDDOmainLabel will be the full
> > domain name, and cert.commonName will be '*.mozqa.com'. The expect.equal
> > does not change that. If I understood what you meant there by 'replacement'
> 
> User expect.match() instead of expect.equal(). But can you show me the
> logging output for that?

Log:

"message": "Web Site label equals '*.mozqa.com' - 'ssl-dv.mozqa.com' should equal '*.mozqa.com'", "lineNumber": 111, "name": "checkSecurityTab", "fileName": "file:///home/vladmaniac/Desktop/testBlueLarry/mozmill-tests/tests/functional/testSecurity/testBlueLarry.js"}}], "name": "testBlueLarry.js::testLarryBlue", "filename": "/home/vladmaniac/Desktop/testBlueLarry/mozmill-tests/tests/functional/testSecurity/testBlueLarry.js", "failed": 1, "passed": 18}

expect line causing the log: 
expect.equal(webIDDomainLabel.getNode().value, cert.commonName, 
             "Web Site label equals '" + cert.commonName + "'");
Comment 19 Henrik Skupin (:whimboo) 2012-06-27 02:58:17 PDT
When I open the cert panel I see: "ssl-dv.mozqa.com" as label. Where does '*.mozqa.com' come from?

Also we should change the testname because we no longer show a blue larry. A name like testDVCert.js would be more appropriate.
Comment 20 Maniac Vlad Florin (:vladmaniac) 2012-06-27 04:55:49 PDT
(In reply to Henrik Skupin (:whimboo) from comment #19)
> When I open the cert panel I see: "ssl-dv.mozqa.com" as label. Where does
> '*.mozqa.com' come from?

cert.commonName has that value

> 
> Also we should change the testname because we no longer show a blue larry. A
> name like testDVCert.js would be more appropriate.

This won't be a problem
Comment 21 Henrik Skupin (:whimboo) 2012-06-27 05:30:44 PDT
(In reply to Maniac Vlad Florin (:vladmaniac) from comment #20)
> (In reply to Henrik Skupin (:whimboo) from comment #19)
> > When I open the cert panel I see: "ssl-dv.mozqa.com" as label. Where does
> > '*.mozqa.com' come from?
> 
> cert.commonName has that value

This can't be given your expect.equal() line you have pointed out above. There this variable is on the 2nd position and correlates to 'ssl-dv.mozqa.com'.
Comment 22 Maniac Vlad Florin (:vladmaniac) 2012-06-27 06:44:02 PDT
(In reply to Henrik Skupin (:whimboo) from comment #21)
> (In reply to Maniac Vlad Florin (:vladmaniac) from comment #20)
> > (In reply to Henrik Skupin (:whimboo) from comment #19)
> > > When I open the cert panel I see: "ssl-dv.mozqa.com" as label. Where does
> > > '*.mozqa.com' come from?
> > 
> > cert.commonName has that value
> 
> This can't be given your expect.equal() line you have pointed out above.
> There this variable is on the 2nd position and correlates to
> 'ssl-dv.mozqa.com'.

Please add 

dump("\n\n webID = " + webIDDomainLabel.getNode().value + "\n\n");
dump("\n\n cert.commonName = " + cert.commonName + "\n\n"); 

Thanks what I did before answering comment 20
Comment 23 Maniac Vlad Florin (:vladmaniac) 2012-06-27 07:47:29 PDT
Created attachment 637119 [details] [diff] [review]
patch v3.0

fixed with expect.match and expect.equal 
no modifications required in strings
Comment 24 Henrik Skupin (:whimboo) 2012-06-27 07:55:35 PDT
Ok my fault. It's not enough to check the page info window but you really have to check the 'view certificate' window. So yeah, use the cert commonName as a regex for the match() method. You will probably have to escape it first to be a regex.
Comment 25 Maniac Vlad Florin (:vladmaniac) 2012-06-27 07:59:21 PDT
(In reply to Henrik Skupin (:whimboo) from comment #24)
> Ok my fault. It's not enough to check the page info window but you really
> have to check the 'view certificate' window. So yeah, use the cert
> commonName as a regex for the match() method. You will probably have to
> escape it first to be a regex.

So my current patch would be wrong, right?
Comment 26 Henrik Skupin (:whimboo) 2012-06-27 08:01:15 PDT
Comment on attachment 637119 [details] [diff] [review]
patch v3.0

>+++ b/tests/functional/testSecurity/testBlueLarry.js

As last step please do not forget to rename the file. Lets do it once the expect call works.

>-  controller.assertValue(webIDDomainLabel, cert.commonName.replace("*", "mail"));
>+  expect.match(webIDDomainLabel.getNode().value, cert.commonName, 
>+               "Expected web site label found");

The regex isn't correct. As mentioned before it has to be escaped first.
Comment 27 Maniac Vlad Florin (:vladmaniac) 2012-06-27 08:18:45 PDT
(In reply to Henrik Skupin (:whimboo) from comment #26)
> Comment on attachment 637119 [details] [diff] [review]
> patch v3.0
> 
> >+++ b/tests/functional/testSecurity/testBlueLarry.js
> 
> As last step please do not forget to rename the file. Lets do it once the
> expect call works.
> 
> >-  controller.assertValue(webIDDomainLabel, cert.commonName.replace("*", "mail"));
> >+  expect.match(webIDDomainLabel.getNode().value, cert.commonName, 
> >+               "Expected web site label found");
> 
> The regex isn't correct. As mentioned before it has to be escaped first.

You're right, on it
Comment 28 Maniac Vlad Florin (:vladmaniac) 2012-06-27 09:39:28 PDT
Created attachment 637162 [details] [diff] [review]
patch v4.0

renamed the file and escaped the string
Comment 29 Henrik Skupin (:whimboo) 2012-06-28 03:48:14 PDT
Comment on attachment 637162 [details] [diff] [review]
patch v4.0

>rename to tests/functional/testSecurity/testDVCert.js

Please use testDVCertificate.js. 

>-  controller.assertValue(webIDDomainLabel, cert.commonName.replace("*", "mail"));
>+  expect.match(webIDDomainLabel.getNode().value, escape(cert.commonName), 
>+               "Expected web site label found");

This is not the escape method you want for a regex. Please run with --show-all and check the message for this assertion. Also please make a negative test before submitting the next version of the patch.
Comment 30 Maniac Vlad Florin (:vladmaniac) 2012-07-02 04:06:16 PDT
(In reply to Henrik Skupin (:whimboo) from comment #29)
> Comment on attachment 637162 [details] [diff] [review]
> patch v4.0
> 
> >rename to tests/functional/testSecurity/testDVCert.js
> 
> Please use testDVCertificate.js. 

Not a problem

> 
> >-  controller.assertValue(webIDDomainLabel, cert.commonName.replace("*", "mail"));
> >+  expect.match(webIDDomainLabel.getNode().value, escape(cert.commonName), 
> >+               "Expected web site label found");
> 
> This is not the escape method you want for a regex. Please run with
> --show-all and check the message for this assertion. Also please make a
> negative test before submitting the next version of the patch.

I am trying to use string.replace, and escape the '*' and '.' characters 

  expect.match(webIDDomainLabel.getNode().value, cert.commonName.replace("*.", "\\*\\."), 
               "Expected web site label found");

I am afraid the output of this line in the console is not the one we want: 

"message": "Expected web site label found - '/(?:)/' matches for 'ssl-dv.mozqa.com'", "lineNumber": 110,

but if I do a simple dump in the console, cert.commonName.replace("*.", "\\*\\.") will echo 
\*\.mozqa.com which is the output we want

What am I doing wrong? Is there some other method you have in mind Henrik?
Comment 31 Henrik Skupin (:whimboo) 2012-07-02 04:09:03 PDT
I would say use some hardcoded strings to check the match() method first. Once that works we know which regex we have to use.
Comment 32 Maniac Vlad Florin (:vladmaniac) 2012-07-02 06:08:47 PDT
Right 

So a passing testcases with match is 
  expect.match("12233344454323", /12233344454323/);
and a failing one is 
  expect.match("abcdef", /12233344454323/);
Comment 33 Maniac Vlad Florin (:vladmaniac) 2012-07-02 06:17:25 PDT
The following line, back in our case 

expect.match(webIDDomainLabel.getNode().value, /.mozqa.com/, 
               "Expected web site label found"); 

will echo the following log with --show-all 

"message": "Expected web site label found - '/.mozqa.com/' matches for 'ssl-dv.mozqa.com'", "lineNumber": 110

I think we are alright now - Henrik?
Comment 34 Henrik Skupin (:whimboo) 2012-07-02 07:14:38 PDT
(In reply to Maniac Vlad Florin (:vladmaniac) from comment #33)
> "message": "Expected web site label found - '/.mozqa.com/' matches for
> 'ssl-dv.mozqa.com'", "lineNumber": 110
> 
> I think we are alright now - Henrik?

This will also match:
* whatevermozqa.com
* amozqaacom.de
* sub.mozqa.com.whatever.fqdn.com

As pointed out a couple of times, do not forget to do negative tests. We really want to test what the certificat offers.
Comment 35 Maniac Vlad Florin (:vladmaniac) 2012-07-02 07:29:19 PDT
I was into assertions.js to check 'match' that I forgot a bit our initial purpose ... 
So we need to separate the exact domain
Comment 36 Maniac Vlad Florin (:vladmaniac) 2012-07-03 05:59:07 PDT
The regex we want for the mozqa.com subdomain is 

/^[^\.]*\.mozqa\.com$/

We can easily test this with the web console adding the following 

/^[^\.]*\.mozqa\.com$/.exec("mozqa.com") echoes null 
/^[^\.]*\.mozqa\.com$/.exec(".mozqa.com") echoes [15:54:59.837] [".mozqa.com"] 
/^[^\.]*\.mozqa\.com$/.exec("something.mozqa.com") echoes [15:56:48.041] ["something.mozqa.com"]
/^[^\.]*\.mozqa\.com$/.exec("whatevermozqa.com") echoes null 
/^[^\.]*\.mozqa\.com$/.exec("amozqaacom.de") echoes null 
/^[^\.]*\.mozqa\.com$/.exec("sub.mozqa.com.whatever.fqdn.com"); echoes null
Comment 37 Maniac Vlad Florin (:vladmaniac) 2012-07-04 06:32:10 PDT
Henrik or Dave, can you please validate this so that I can upload a fix patch for this test?
Comment 38 Henrik Skupin (:whimboo) 2012-07-04 06:35:35 PDT
(In reply to Maniac Vlad Florin (:vladmaniac) from comment #36)
> The regex we want for the mozqa.com subdomain is 
> 
> /^[^\.]*\.mozqa\.com$/

This would not work if you are using multiple subdomains like a.b.c.mozqa.com. The wildcard should IMHO match for all of those.
Comment 39 Maniac Vlad Florin (:vladmaniac) 2012-07-04 07:18:41 PDT
(In reply to Henrik Skupin (:whimboo) from comment #38)
> (In reply to Maniac Vlad Florin (:vladmaniac) from comment #36)
> > The regex we want for the mozqa.com subdomain is 
> > 
> > /^[^\.]*\.mozqa\.com$/
> 
> This would not work if you are using multiple subdomains like
> a.b.c.mozqa.com. The wildcard should IMHO match for all of those.

I intentionally ruled out that 
If we want a.b.c.mozqa.com to work the regex is 

--
[17:17:58.028] /^.*\.mozqa\.com$/.exec("a.b.c.mozqa.com")
[17:17:58.070] ["a.b.c.mozqa.com"]
Comment 40 Henrik Skupin (:whimboo) 2012-07-04 07:30:47 PDT
Right. Now you only have to transform the value from cert.commonName to be '/^.*\.mozqa\.com$/'.
Comment 41 Maniac Vlad Florin (:vladmaniac) 2012-07-05 02:24:32 PDT
Created attachment 639270 [details] [diff] [review]
patch v5.0

Regex identified and build successfully 
However if we look into the expect.match log messages we will see an extra "\" added which is abnormal and I will investigate this further most certainly in another bug. 
I thought so far I was doing things wrong but playing with the expect.match method in the scratchpad everything seems fine so I suspect something in the log method to be not in place. 

Asking for r from Henrik only since he did it in the last patch
Comment 42 Henrik Skupin (:whimboo) 2012-07-05 03:10:35 PDT
Comment on attachment 639270 [details] [diff] [review]
patch v5.0

>-                                            "security-identity-domain-value");
>+                                            "security-identity-domain-value"); 

nit: please remove the trailing blank.

>+  var certNameToRegExp = new RegExp("^." + cert.commonName.replace(/\./g, "\\\.") + "$");

That's not enough. You will also have to replace any '*' with '.*'. You shouldn't do this by simply adding the '.' to the prefix string. Also for replace() I would not use a regex but a simple string.
Comment 43 Maniac Vlad Florin (:vladmaniac) 2012-07-05 04:51:30 PDT
 Also for
> replace() I would not use a regex but a simple string.

I am not successful in using replace() with a simple string due to the fact that we need to replace all the "." with "\." (escaped) so that the RegExp will know that '.' is not a special character. For that I need the regex imo.
Comment 44 Henrik Skupin (:whimboo) 2012-07-05 05:00:02 PDT
How do you replace? If you have problems please always add the lines of code in the comment. It's not helpful when you lack this information.

But ok, seems like you have to use a regex. At least when checking MDN. So just run the replace twice, first for '.' and then for the '*' chars.
Comment 45 Maniac Vlad Florin (:vladmaniac) 2012-07-05 05:30:08 PDT
Created attachment 639299 [details] [diff] [review]
patch v5.1

All requests addressed
Comment 46 Henrik Skupin (:whimboo) 2012-07-09 13:01:42 PDT
Comment on attachment 639299 [details] [diff] [review]
patch v5.1

>-  controller.assertValue(webIDDomainLabel, cert.commonName.replace("*", "mail"));
>+  var certNameToRegExp = new RegExp("^" + (cert.commonName.replace(/\./g, "\\\.")).replace("*", ".*") + "$");

This line is longer than 80 chars. Please break it into the next line or do it in two separate statements. Also try to find some shorter variable names. Once this is done we are good to land.
Comment 47 Maniac Vlad Florin (:vladmaniac) 2012-07-09 23:56:55 PDT
Created attachment 640521 [details] [diff] [review]
patch v5.2

Fixed.

Please note that there are many other lines in the test longer than 80 char. Should we split those also? 

Also the same for the variable names
Comment 48 Henrik Skupin (:whimboo) 2012-07-11 07:06:23 PDT
Comment on attachment 640521 [details] [diff] [review]
patch v5.2

>-  controller.assertValue(webIDDomainLabel, cert.commonName.replace("*", "mail"));
>+  var certName = (cert.commonName.replace(/\./g, "\\\.")).replace("*", ".*");

Sorry, one more thing. Please make both replace calls work on regex. The second one should also be global.

>+  var certNameRegExp = new RegExp("^" + certName + "$");

AFAIR you can directly pass in the string too. There shouldn't be the need to create a RegExp instance first. Please try it out.
Comment 49 Maniac Vlad Florin (:vladmaniac) 2012-07-12 07:58:45 PDT
Created attachment 641468 [details] [diff] [review]
patch v5.3

All changes addressed. Hope I understood correctly what we want here 
Also, removed the /XXX comment it seems we missed that. We are no longer doing what the comment said
Comment 50 Henrik Skupin (:whimboo) 2012-07-12 08:13:20 PDT
Comment on attachment 641468 [details] [diff] [review]
patch v5.3

>-  controller.assertValue(webIDDomainLabel, cert.commonName.replace("*", "mail"));
>+  var certName = (cert.commonName.replace(/\./g, "\\\.")).replace(/\*/g, ".*");

When you can replace '*' with a string can we do the same with '.'? I think we should be consistent here.

>+  var certName = "^" + certName + "$";
>+
>+  expect.match(webIDDomainLabel.getNode().value, certName, 

Just do the string concatenation directly in the match() call.
Comment 51 Maniac Vlad Florin (:vladmaniac) 2012-07-13 00:05:54 PDT
(In reply to Henrik Skupin (:whimboo) from comment #50)
> Comment on attachment 641468 [details] [diff] [review]
> patch v5.3
> 
> >-  controller.assertValue(webIDDomainLabel, cert.commonName.replace("*", "mail"));
> >+  var certName = (cert.commonName.replace(/\./g, "\\\.")).replace(/\*/g, ".*");
> 
> When you can replace '*' with a string can we do the same with '.'? I think
> we should be consistent here.
> 
Not sure what you mean here but we replace "." with "\." in the first replace cert.commonName.replace(/\./g, "\\\."), then we replace the "*" with ".*"
Comment 52 Maniac Vlad Florin (:vladmaniac) 2012-07-13 07:23:30 PDT
> >+  var certNameRegExp = new RegExp("^" + certName + "$");
> 
> AFAIR you can directly pass in the string too. There shouldn't be the need
> to create a RegExp instance first. Please try it out.

Also please try this out yourself and look into the match() log message. We need the regex to get the proper log message, instead, we get 

Expected web site label found - '/(?:)/' matches for 'ssl-dv.mozqa.com'" 

which is not right. So I propose sticking to the regex
Comment 53 Henrik Skupin (:whimboo) 2012-07-16 00:09:09 PDT
(In reply to Maniac Vlad Florin (:vladmaniac) from comment #52)
> > AFAIR you can directly pass in the string too. There shouldn't be the need
> > to create a RegExp instance first. Please try it out.
> 
> Also please try this out yourself and look into the match() log message. We
> need the regex to get the proper log message, instead, we get 
> 
> Expected web site label found - '/(?:)/' matches for 'ssl-dv.mozqa.com'" 

As I have said you should try this before submitting a new patch for review. Looks like this didn't happen. Also we don't want to try out each individual change in the patch. There should be a trust in writing and fixing testcases, where you as patch author test the correctness.

(In reply to Maniac Vlad Florin (:vladmaniac) from comment #51)
> Not sure what you mean here but we replace "." with "\." in the first
> replace cert.commonName.replace(/\./g, "\\\."), then we replace the "*" with
> ".*"

Ok, we can leave that. The three backslashes are just an escaped backslash for the special handling of the dot.
Comment 54 Maniac Vlad Florin (:vladmaniac) 2012-07-16 00:27:34 PDT
Created attachment 642497 [details] [diff] [review]
patch v5.4

fixed
Comment 55 Henrik Skupin (:whimboo) 2012-07-16 03:19:16 PDT
Comment on attachment 642497 [details] [diff] [review]
patch v5.4

Finally it looks good and works. I will land on default now.
Comment 56 Henrik Skupin (:whimboo) 2012-07-16 03:52:37 PDT
Pushed to default:
http://hg.mozilla.org/qa/mozmill-tests/rev/c98e20ff0951
Comment 57 Henrik Skupin (:whimboo) 2012-07-16 15:32:53 PDT
Given that we had no new failure today I have landed the patch on the other branches.

http://hg.mozilla.org/qa/mozmill-tests/rev/abbd49d6d021 (beta)
http://hg.mozilla.org/qa/mozmill-tests/rev/7799f37de1b0 (release)

Vlad, please come up with a patch for esr10. Also once it has been landed please update the Litmus test references.
Comment 58 Maniac Vlad Florin (:vladmaniac) 2012-07-16 23:58:50 PDT
Created attachment 642874 [details] [diff] [review]
[mozilla-esr10] patch v1.0

Patch for esr branch
Comment 59 Maniac Vlad Florin (:vladmaniac) 2012-07-17 00:01:37 PDT
Was tested also with the latest candidate build from ftp://ftp.mozilla.org/pub/firefox/nightly/10.0.6esr-candidates/build1/linux-i686/en-US/
Comment 60 Henrik Skupin (:whimboo) 2012-07-17 02:30:58 PDT
Pushed to esr10:
http://hg.mozilla.org/qa/mozmill-tests/rev/cbd547036384
Comment 61 Maniac Vlad Florin (:vladmaniac) 2012-07-17 04:07:54 PDT
Really tried to find all the branches in litmus but it seems to me that we are missing default and beta.  

Modified for aurora and release branch: 
release: https://litmus.mozilla.org/show_test.cgi?id=64707
aurora: https://litmus.mozilla.org/show_test.cgi?id=16360

Note: the test was not disabled, its just the mozmill test name changed so updated that info in litmus
Comment 62 Henrik Skupin (:whimboo) 2012-07-17 04:47:27 PDT
(In reply to Maniac Vlad Florin (:vladmaniac) from comment #61)
> Really tried to find all the branches in litmus but it seems to me that we
> are missing default and beta.  

We do not have tests for Nightly in Litmus. And beta is the 14.0 branch.

> Modified for aurora and release branch: 
> release: https://litmus.mozilla.org/show_test.cgi?id=64707
> aurora: https://litmus.mozilla.org/show_test.cgi?id=16360

You should also update the 13.0 branch even it gets obsoleted later today.
Comment 63 Maniac Vlad Florin (:vladmaniac) 2012-07-17 04:55:43 PDT
Modifications for Firefox 13 were made 

https://litmus.mozilla.org/show_test.cgi?id=56353
Comment 64 Henrik Skupin (:whimboo) 2012-07-25 04:52:40 PDT
The attached patch missed to update the manifest file, so Mozmill 2.0 is broken in running the tests.

I had to land this immediately because it's blocking my work for bug 767332:

http://hg.mozilla.org/qa/mozmill-tests/rev/bc76dc7bed67 (default)
http://hg.mozilla.org/qa/mozmill-tests/rev/1f7a4d048006 (aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/62937f290a30 (beta)
http://hg.mozilla.org/qa/mozmill-tests/rev/6bf78d6cd2b7 (release)
http://hg.mozilla.org/qa/mozmill-tests/rev/86a076f99fab (esr10)
Comment 66 Maniac Vlad Florin (:vladmaniac) 2012-08-13 04:44:23 PDT
Thanks Ioana!

Note You need to log in before you can comment on or make changes to this bug.