708491 - testSecurityNotification.js fails due to timeout on cert_domain_link

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect, P2)

Description

[u279076]

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)]

Attachment: skip test (all branches)

Skips the test.

Comment 2

[Maniac Vlad Florin (:vladmaniac)]

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)]

This is related to bug 708494 because of the webpage we use to test security elements.

Comment 4

[u279076]

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

[u279076]

Attachment: disable test (all) [checked-in]

Comment 6

[u279076]

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

[u279076]

Attachment: disable test (1.9.2) [checked-in]

Comment 8

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

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

[u279076]

(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][⌚️UTC+2]]

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

[u279076]

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

[u279076]

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]

I am working on this bug to fix

Comment 14

[Sidharth Chugh]

I am working on this bug to fix

Comment 15

[u279076]

Thanks Sidharth. Please let me know via email ([e-mail]) or IRC (ashughes) if you need help getting started.

Comment 16

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

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][⌚️UTC+2]]

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

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

Attachment: 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] [he/him] ⌚BST]

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

Attachment: 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][⌚️UTC+2]]

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][⌚️UTC+2]]

http://hg.mozilla.org/qa/mozmill-tests/rev/a742c618fc98 (default)

Comment 24

[Andreea Matei [:AndreeaMatei]]

I've checked and this patch applies cleanly to all other branches.

Comment 25

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

http://hg.mozilla.org/qa/mozmill-tests/rev/ddbbb1cf51d5 (aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/ed29839958fb (beta)
http://hg.mozilla.org/qa/mozmill-tests/rev/9c2386fe032c (release)
http://hg.mozilla.org/qa/mozmill-tests/rev/054004a8aba1 (esr10)