Last Comment Bug 708491 - testSecurityNotification.js fails due to timeout on cert_domain_link
: testSecurityNotification.js fails due to timeout on cert_domain_link
Status: RESOLVED FIXED
[mozmill-test-failure] s=121015 u=fai...
:
Product: Mozilla QA
Classification: Other
Component: Mozmill Tests (show other bugs)
: unspecified
: All All
: P2 normal (vote)
: ---
Assigned To: Andreea Matei [:AndreeaMatei]
:
:
Mentors:
http://mozmill-release.brasstacks.moz...
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-12-07 17:36 PST by Anthony Hughes (:ashughes) [GFX][QA][Mentor]
Modified: 2012-10-25 09:16 PDT (History)
7 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---
fixed
fixed
fixed
fixed
fixed


Attachments
skip test (all branches) (2.18 KB, patch)
2011-12-08 07:55 PST, Remus Pop (:RemusPop)
vlad.mozbugs: review+
anthony.s.hughes: review+
Details | Diff | Splinter Review
disable test (all) [checked-in] (2.20 KB, patch)
2011-12-08 10:04 PST, Anthony Hughes (:ashughes) [GFX][QA][Mentor]
anthony.s.hughes: review+
Details | Diff | Splinter Review
disable test (1.9.2) [checked-in] (1.31 KB, patch)
2011-12-08 10:15 PST, Anthony Hughes (:ashughes) [GFX][QA][Mentor]
hskupin: review+
Details | Diff | Splinter Review
patch v1 (3.53 KB, patch)
2012-09-21 07:12 PDT, Andreea Matei [:AndreeaMatei]
dave.hunt: review-
Details | Diff | Splinter Review
patch v2 (3.63 KB, patch)
2012-10-22 05:24 PDT, Andreea Matei [:AndreeaMatei]
hskupin: review+
Details | Diff | Splinter Review

Description Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-12-07 17:36:02 PST
The following test failure was noticed today during the 9.0b5 test runs across all platforms:

Filename: /testSecurity/testSecurityNotification.js
Test: testSecurityNotification.js :: testSecNotification
Error: Timeout exceeded for waitForElementID: cert_domain_link
Comment 1 Remus Pop (:RemusPop) 2011-12-08 07:55:55 PST
Created attachment 580044 [details] [diff] [review]
skip test (all branches)

Skips the test.
Comment 2 Maniac Vlad Florin (:vladmaniac) 2011-12-08 07:57:08 PST
Comment on attachment 580044 [details] [diff] [review]
skip test (all branches)

Same as for the other skip patch - remember to come back on this one. Thanks! :)
Comment 3 Remus Pop (:RemusPop) 2011-12-08 07:58:04 PST
This is related to bug 708494 because of the webpage we use to test security elements.
Comment 4 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-12-08 10:01:55 PST
Comment on attachment 580044 [details] [diff] [review]
skip test (all branches)

Please remember to hand off reviews to me for check-in. Also, your commit message is worded backwards. I will have to upload a fixed patch.
Comment 5 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-12-08 10:04:36 PST
Created attachment 580085 [details] [diff] [review]
disable test (all) [checked-in]
Comment 6 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-12-08 10:10:01 PST
Comment on attachment 580085 [details] [diff] [review]
disable test (all) [checked-in]

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/eacfdfac2cad (default)
http://hg.mozilla.org/qa/mozmill-tests/rev/9bc4bc1a4926 (mozilla-aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/eef3503363a9 (mozilla-beta)
http://hg.mozilla.org/qa/mozmill-tests/rev/6d52b1e6d76f (mozilla-release)

Patch needed to disable this test on mozilla-1.9.2
Comment 7 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-12-08 10:15:18 PST
Created attachment 580090 [details] [diff] [review]
disable test (1.9.2) [checked-in]
Comment 8 Henrik Skupin (:whimboo) [away 09/30 - 10/06] 2011-12-08 12:32:52 PST
Comment on attachment 580090 [details] [diff] [review]
disable test (1.9.2) [checked-in]

Usually we should only add the skipping code here. Otherwise it could cause issues with the merge process if skipping a test only applies to some but not all branches. Please change that. Otherwise r=me.
Comment 9 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-12-08 13:59:08 PST
(In reply to Henrik Skupin (:whimboo) from comment #8)
> Comment on attachment 580090 [details] [diff] [review]
> disable test (1.9.2)
> 
> Usually we should only add the skipping code here. Otherwise it could cause
> issues with the merge process if skipping a test only applies to some but
> not all branches. Please change that. Otherwise r=me.

We've been in the habit of removing the deprecated "Litmus" lines for some time now. The all-branches patch above includes it. Also, we will never merge 1.9.2 into one of the rapid release channels, will we?

Given that, I think this patch is fine to check in as is; do you disagree?
Comment 10 Henrik Skupin (:whimboo) [away 09/30 - 10/06] 2011-12-08 15:13:19 PST
Given that we possibly will not backout this patch go ahead. I just wanted to add this information for the future, so it's clear when we have such a case.
Comment 11 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-12-08 16:16:52 PST
Comment on attachment 580090 [details] [diff] [review]
disable test (1.9.2) [checked-in]

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/3f0b9f114a84 (mozilla-1.9.2)
Comment 12 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-01-27 14:21:41 PST
I think the problem with this test is that https://mozilla.org and http://www.mozilla.org are no longer valid test sites (the certs have been fixed).
Comment 13 Sidharth Chugh 2012-03-15 06:36:24 PDT
I am working on this bug to fix
Comment 14 Sidharth Chugh 2012-03-15 06:37:54 PDT
I am working on this bug to fix
Comment 15 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-03-22 14:24:35 PDT
Thanks Sidharth. Please let me know via email (ahughes@mozilla.com) or IRC (ashughes) if you need help getting started.
Comment 16 Henrik Skupin (:whimboo) [away 09/30 - 10/06] 2012-06-05 04:11:37 PDT
Sidharth, we haven't heard from you for a longer time. Are you still ok with working on a fix for this failure?
Comment 17 Henrik Skupin (:whimboo) [away 09/30 - 10/06] 2012-09-07 16:19:40 PDT
Given that Sidharth sadly didn't get back to us we have to move forward on this test failure and get the test re-enabled. Putting it on our list for the next week.
Comment 18 Andreea Matei [:AndreeaMatei] 2012-09-20 02:00:28 PDT
It looks like we fail because the last loaded page (https://mozilla.org/) should be one with an invalid certificate so the buttons "Get me out of here" and "Add Exception" be present. 
Instead, that page is a secure one, with Green Larry.

I have tested with an unsecure page, but it still fails at finding the "cert_domain_link" ID. 
It appears that we only have the technicalContentText which contains the whole text from Technical Details. 
So I propose we use that ID and an expect.contain() to check the link.

Another thing I've noticed in this test is that the first loaded page (https://addons.mozilla.org/licenses/5.txt) is a "Not Found" page, but it is a secure one so there's no failure there.
Comment 19 Andreea Matei [:AndreeaMatei] 2012-09-21 07:12:39 PDT
Created attachment 663385 [details] [diff] [review]
patch v1

Removed time constants that are unnecessary, added constants for the visited pages and as discussed in "Ask the experts" meeting and on IRC, used "https://summitbook.mozilla.org/" for the invalid cert page as well as expect.match() for checking the correct link.
Comment 20 Dave Hunt (:davehunt) 2012-09-24 03:34:27 PDT
Comment on attachment 663385 [details] [diff] [review]
patch v1

Review of attachment 663385 [details] [diff] [review]:
-----------------------------------------------------------------

::: tests/functional/testSecurity/testSecurityNotification.js
@@ +37,3 @@
>    controller.waitForPageLoad();
>  
>    // Verify the link in Technical Details is correct

This test is now using an expired certificate rather than a mismatching one, so there is no link in the page. See http://mxr.mozilla.org/mozilla-central/source/browser/components/certerror/content/aboutCertError.xhtml#130

@@ +38,5 @@
>  
>    // Verify the link in Technical Details is correct
> +  var text = new elementslib.ID(controller.tabs.activeTab, "technicalContentText");
> +  controller.waitForElement(text);
> +  expect.match(text.getNode().textContent, /([-\w\.]+)+?/,

This looks like a very loose match. If we want to check that the page URL appears in the text we should use INVALID_CERT_PAGE.
Comment 21 Andreea Matei [:AndreeaMatei] 2012-10-22 05:24:37 PDT
Created attachment 673833 [details] [diff] [review]
patch v2

Updated accordingly to the review, used the invalid certificate page and substring to get rid of "http://" when checking the Technical details.
Comment 22 Henrik Skupin (:whimboo) [away 09/30 - 10/06] 2012-10-22 16:29:27 PDT
Comment on attachment 673833 [details] [diff] [review]
patch v2

Review of attachment 673833 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me. Please check that we can backport the patch.
Comment 23 Henrik Skupin (:whimboo) [away 09/30 - 10/06] 2012-10-22 16:31:44 PDT
http://hg.mozilla.org/qa/mozmill-tests/rev/a742c618fc98 (default)
Comment 24 Andreea Matei [:AndreeaMatei] 2012-10-23 01:19:57 PDT
I've checked and this patch applies cleanly to all other branches.

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