Closed Bug 1304004 Opened 8 years ago Closed 8 years ago

Permafail test_dv_certificate.py, test_ev_certificate.py, test_mixed_content_page.py, test_mixed since bug 1303291 landed

Categories

(Testing :: Firefox UI Tests, defect)

Product:
Testing
Component:
Firefox UI Tests
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox51 fixed, firefox52 fixed, firefox53 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox51 --- fixed
firefox52 --- fixed
firefox53 --- fixed

People

(Reporter: aryx, Assigned: whimboo)

References

Details

(Keywords: regression)

Attachments

(1 file)

Bug 1304004 - Fix regression in firefox-ui security tests caused by bug 1303291.
58 bytes, text/x-review-board-request
keeler
: review+
Details 
Fallout from bug 1303291:

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=42a77283ee64b4528b054104fa75f90fbbcfb515
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=36134916&repo=mozilla-inbound


TEST-UNEXPECTED-FAIL | test_dv_certificate.py TestDVCertificate.test_dv_cert | AssertionError: 'identity-secure' not found in u'url("chrome://browser/skin/connection-secure.svg")' 

This is currently permafailing.
Blocks: 1303291
Keywords: regression
Summary: TEST-UNEXPECTED-FAIL | test_dv_certificate.py TestDVCertificate.test_dv_cert | AssertionError: 'identity-secure' not found in u'url("chrome://browser/skin/connection-secure.svg")' since bug 1303291 landed → Permafail test_dv_certificate.py TestDVCertificate.test_dv_cert | AssertionError: 'identity-secure' not found in u'url("chrome://browser/skin/connection-secure.svg")' since bug 1303291 landed
Flags: needinfo?(dao+bmo)
So this test tries to match part of the icon name. That's bad practice, because:

1. Icon names aren't stable, we change them all the time.

2. Had you used "identity-secure.svg" rather than just "identity-secure", I'd at least have had a chance to catch this.

Can you please fix this test?
Flags: needinfo?(dao+bmo) → needinfo?(hskupin)
Sorry, I cannot fix it due to time constraints. But I'm happy to review a patch. Please keep in mind that several tests are affected.
Flags: needinfo?(hskupin)
dao> Aryx: so am I supposed to fix this (bug 1304004)? I could patch up the test easily enough, but the test is kind of stupid and should ideally be rewritten. whimboo doesn't have time for that, and I'm not familiar with the test framework. the test also seems to have landed without browser peer review. should I disable it?
Aryx> dao: sounds like the best solution

So I'm going to disable these tests.
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/061f5e9d34eb
Disable test_dv_certificate.py, test_ev_certificate.py, test_mixed_content_page.py, test_mixed_script_content_blocking.py
Keywords: leave-open
Whiteboard: tests disabled
Summary: Permafail test_dv_certificate.py TestDVCertificate.test_dv_cert | AssertionError: 'identity-secure' not found in u'url("chrome://browser/skin/connection-secure.svg")' since bug 1303291 landed → Permafail test_dv_certificate.py, test_ev_certificate.py, test_mixed_content_page.py, test_mixed since bug 1303291 landed
Dao, so when you checked those tests what specifically would have to be updated? It would be great to know that so we can re-enable the tests. Simply disabling them is not a great idea, especially with the excuse to not know something about the tests. They are documented on MDN. Also regressions as introduced should simply be fixed.
Flags: needinfo?(dao+bmo)
Dao, it would be great to get your feedback here. Just in the case that I might have some spare time by next week and could have a look at those tests. Thanks.
(In reply to Dão Gottwald [:dao] from comment #3)
> dao> Aryx: so am I supposed to fix this (bug 1304004)? I could patch up the
> test easily enough, but the test is kind of stupid and should ideally be
> rewritten. whimboo doesn't have time for that, and I'm not familiar with the
> test framework. the test also seems to have landed without browser peer
> review. should I disable it?
> Aryx> dao: sounds like the best solution
> 
> So I'm going to disable these tests.

This appears to have been disabled without speaking to the owner of the tests nor seem to have been reviewed by the peers in Testing. This is grounds for a backout on improper review. Please can we get the answers on the need-info ASAP.
Flags: needinfo?(dolske)
I can only speculate as to why these tests were written this way, but I would suggest that you make them just check the class names on the identity block. We usually don't check icons directly and I'm not sure why we should do that here. I don't think it adds significant value.
Flags: needinfo?(dao+bmo)
(In reply to Dão Gottwald [:dao] from comment #10)
> block. We usually don't check icons directly and I'm not sure why we should
> do that here. I don't think it adds significant value.

The changes as landed by yourself include also test changes which still check the images:
https://hg.mozilla.org/mozilla-central/diff/42a77283ee64/browser/base/content/test/general/head.js#l1.13

If you think that such a check for a domain with a real SSL certificate should not be done, then let me know. I have concerns with your proposal at least for EV, which cannot be tested in mochitests or browser chrome tests. At least that is what I got from David Keeler and why we want to keep the remote EV related tests.
(In reply to Henrik Skupin (:whimboo) from comment #11)
> (In reply to Dão Gottwald [:dao] from comment #10)
> > block. We usually don't check icons directly and I'm not sure why we should
> > do that here. I don't think it adds significant value.
> 
> The changes as landed by yourself include also test changes which still
> check the images:
> https://hg.mozilla.org/mozilla-central/diff/42a77283ee64/browser/base/
> content/test/general/head.js#l1.13

Indeed, I think those tests are overzealous too. It was easy enough for me to update them since they included the full file names rather than a substring, but I still think it's a bad idea.
(In reply to Dão Gottwald [:dao] from comment #12)
> Indeed, I think those tests are overzealous too. It was easy enough for me
> to update them since they included the full file names rather than a
> substring, but I still think it's a bad idea.

Ok, so lets do the same for our tests this time. Soon we will rewrite the tests to be pure Marionette tests and by that time we can check which parts are not necessary and can be removed.
Assignee: nobody → hskupin
Status: NEW → ASSIGNED
Comment on attachment 8802108 [details]
Bug 1304004 - Fix regression in firefox-ui security tests caused by bug 1303291.

As I said I'd prefer not checking the icon file names. It bakes too much implementation detail into the tests. If somebody else thinks this is worthwhile, please get their review on this.
Attachment #8802108 - Flags: review?(dao+bmo)
Attachment #8802108 - Flags: review?(dkeeler)
Flags: needinfo?(dolske)
Comment on attachment 8802108 [details]
Bug 1304004 - Fix regression in firefox-ui security tests caused by bug 1303291.

https://reviewboard.mozilla.org/r/86634/#review85654

I think if using the full icon url works for browser/base/content/test/general/head.js it should be fine here as well.

::: testing/firefox-ui/tests/functional/security/test_mixed_content_page.py:31
(Diff revision 1)
>  
> -        self.assertIn('identity-mixed-passive-loaded',
> -                      self.locationbar.connection_icon.value_of_css_property('list-style-image'))
> +        # The correct lock icon should be shown
> +        icon = self.locationbar.connection_icon
> +        self.assertEqual('url("chrome://browser/skin/connection-mixed-passive-loaded.svg#icon")',
> +                         icon.value_of_css_property('list-style-image'))
>  

I would add a case for self.locationbar.identity_box.get_attribute('className') like test_dv_certificate.py and test_ev_certificate.py have.

::: testing/firefox-ui/tests/functional/security/test_mixed_script_content_blocking.py:53
(Diff revision 1)
>          connection_icon = self.locationbar.connection_icon
>          Wait(self.marionette, timeout=self.browser.timeout_page_load).until(
>              lambda _: icon_filename in connection_icon.value_of_css_property('list-style-image'),
>              message="The correct icon is displayed"
>          )
>  

Maybe include a test for self.locationbar.identity_box.get_attribute('className') here as well?
Attachment #8802108 - Flags: review?(dkeeler) → review+
David, would you mind having a second look at the patch? I got your requested changes added, but would like to get your feedback on those before landing the patch. Thanks.
Flags: needinfo?(dkeeler)
Comment on attachment 8802108 [details]
Bug 1304004 - Fix regression in firefox-ui security tests caused by bug 1303291.

https://reviewboard.mozilla.org/r/86634/#review86078

Yep, this looks good. Just the one question.

::: testing/firefox-ui/tests/functional/security/test_mixed_script_content_blocking.py:53
(Diff revisions 1 - 2)
>  
>          # First call to Wait() needs a longer timeout due to the reload of the web page.
>          connection_icon = self.locationbar.connection_icon
>          Wait(self.marionette, timeout=self.browser.timeout_page_load).until(
> -            lambda _: icon_filename in connection_icon.value_of_css_property('list-style-image'),
> -            message="The correct icon is displayed"
> +            lambda _: connection_icon.value_of_css_property('list-style-image') == icon_filename,
> +            message='Connection icon "{}" is not set'.format(icon_filename)

Hmmm - is this the message for a failure or a message for success? (In the original file, it looks like the message for success, but this looks like it changes it to a failure message.)
Comment on attachment 8802108 [details]
Bug 1304004 - Fix regression in firefox-ui security tests caused by bug 1303291.

https://reviewboard.mozilla.org/r/86634/#review86078

> Hmmm - is this the message for a failure or a message for success? (In the original file, it looks like the message for success, but this looks like it changes it to a failure message.)

This message will be shown in case of a failure. The former message value was simply wrong. Sorry that I didn't mention it.
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/005f4ce1bf67
Fix regression in firefox-ui security tests caused by bug 1303291. r=keeler
test-only change which fixes a regression. We would also like to have it on aurora. Thanks.
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox51: --- → disabled
status-firefox52: --- → fixed
Keywords: leave-open
Resolution: --- → FIXED
Whiteboard: tests disabled → [checkin-needed-aurora]
Failures here are:

TEST-UNEXPECTED-FAIL | test_mixed_content_page.py TestMixedContentPage.test_mixed_content | AssertionError: 'url("chrome://browser/skin/connection-mixed-passive-loaded.svg#icon")' != u'url("chrome://browser/skin/connection-mixed-passive-loaded.svg#icon-white")'

TEST-UNEXPECTED-ERROR | test_mixed_script_content_blocking.py TestMixedScriptContentBlocking.test_mixed_content_page | TimeoutException: Timed out after 30.0 seconds with message: Connection icon "url("chrome://browser/skin/connection-mixed-active-loaded.svg#icon")" is not set

The problem we are facing on Aurora is that we run with the dark devtools theme and therefore hit the following CSS rule:

> +:root[devtoolstheme="dark"] #connection-icon:-moz-lwtheme

Dao, I assume for tests we should always use the default theme, or? This would be a change we will have to do in Marionette itself.
Flags: needinfo?(hskupin) → needinfo?(dao+bmo)
The dark devtools theme is the default on aurora, so that's what we're running tests with.
Flags: needinfo?(dao+bmo)
(In reply to Dão Gottwald [:dao] from comment #27)
> The dark devtools theme is the default on aurora, so that's what we're
> running tests with.

So what I do not understand then is how the browser chrome tests which you have updated over on bug 1303291 with the exactly same icon URLs work and do not show the mismatch given the different CSS rules for the devtools theme. Do you have an explanation for that? I would kinda appreciate that. Thanks.
Flags: needinfo?(dao+bmo)
I don't know. I just did a dumb search-and-replace on those tests. I would again recommend just not checking images in tests like this.
Flags: needinfo?(dao+bmo)
(In reply to Dão Gottwald [:dao] from comment #29)
> I don't know. I just did a dumb search-and-replace on those tests. I would
> again recommend just not checking images in tests like this.

Looks like this will be the only viable option now. I also don't have the time to investigate Mochitests.

Carsten, can you please get 005f4ce1bf67 backed out from central? Thanks.
Flags: needinfo?(cbook)
(In reply to Henrik Skupin (:whimboo) from comment #30)
> (In reply to Dão Gottwald [:dao] from comment #29)
> > I don't know. I just did a dumb search-and-replace on those tests. I would
> > again recommend just not checking images in tests like this.
> 
> Looks like this will be the only viable option now. I also don't have the
> time to investigate Mochitests.
> 
> Carsten, can you please get 005f4ce1bf67 backed out from central? Thanks.

np, backed out in https://hg.mozilla.org/mozilla-central/rev/523d47a7f8f38c271a2d5cf4fb9cbdbad1036e3b
Status: RESOLVED → REOPENED
Flags: needinfo?(cbook)
Resolution: FIXED → ---
Sorry, but I nearly forgot about this bug...

So given that we have issues with the list style image for developer builds, and I'm not sure how the Mochitest make the tests passing, I would suggest we get the image checks removed. David, can you think of something else we could check for that icon? If not we will loose any coverage for it.
status-firefox52: fixed → affected
Flags: needinfo?(dkeeler)
It seems like we have good coverage for showing the correct icon in the mixed content blocker tests, so maybe let's just drop checking for the icon url/class (but keep the identity box css class checking).
Flags: needinfo?(dkeeler)
(In reply to David Keeler [:keeler] (use needinfo?) from comment #33)
> It seems like we have good coverage for showing the correct icon in the
> mixed content blocker tests, so maybe let's just drop checking for the icon
> url/class (but keep the identity box css class checking).

Sounds fine. I will get those lines removed and pushed to autoland again. I believe I can carry the r+ forward given that this is only about extra code removal.
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9ba35f0c9fea
Fix regression in firefox-ui security tests caused by bug 1303291. r=keeler
https://hg.mozilla.org/mozilla-central/rev/9ba35f0c9fea
Status: REOPENED → RESOLVED
Closed: 8 years ago → 8 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Whiteboard: [checkin-needed-aurora][checkin-needed-beta]
https://hg.mozilla.org/releases/mozilla-aurora/rev/e31e39304343
status-firefox52: affected → fixed
Whiteboard: [checkin-needed-aurora][checkin-needed-beta] → [checkin-needed-beta]
As of today I stumbled about a preference setting which clarifies the question why the Mochitests work just fine on Firefox DevEdition. The reason is that the pref lightweightThemes.selectedThemeID gets set to an empty string for all kind of tests by:

https://dxr.mozilla.org/mozilla-central/source/testing/profiles/prefs_general.js#332

As result we do not run our Mochitests via developer theme but the default one!
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: