Closed Bug 1368143 Opened 4 years ago Closed 4 years ago

Intermittent browser/base/content/test/siteIdentity/browser_bug902156.js | Expected state for activeBlocked matches UI state - Got true, expected false

Categories

(Firefox :: Site Identity, defect, P1)

defect

Tracking

()

RESOLVED FIXED
Firefox 57
Tracking Status
firefox57 --- fixed

People

(Reporter: intermittent-bug-filer, Assigned: johannh)

Details

(Keywords: intermittent-failure, Whiteboard: [stockwell fixed:other])

Attachments

(1 file)

Priority: -- → P3
:johann, this test is failing often on osx, is it possibly you could take a look at fixing this sometime in the next week or two?
Flags: needinfo?(jhofmann)
Whiteboard: [stockwell needswork]
This smells like bug 1348549 again. Only this test is a lot bigger :/

I'd like to note that I'm not the author of either test, but I guess I can fix this one as well.
Assignee: nobody → jhofmann
Status: NEW → ASSIGNED
Flags: needinfo?(jhofmann) → qe-verify-
Priority: P3 → P1
Comment on attachment 8894495 [details]
Bug 1368143 - Modernize browser_bug902156.js.

https://reviewboard.mozilla.org/r/165664/#review172090

Thanks! Tentative r+, with a couple of questions. Sorry for the delay reviewing this!

::: browser/base/content/test/siteIdentity/browser_bug902156.js:52
(Diff revision 2)
> -  BrowserTestUtils.waitForCondition(
> +      await ContentTaskUtils.waitForCondition(
> -    () => content.document.getElementById("mctestdiv").innerHTML == expected,
> +        () => content.document.getElementById("mctestdiv").innerHTML == expected,
> -    "Error: Waited too long for mixed script to run in Test 1B").then(test1C);
> +        "Error: Waited too long for mixed script to run in Test 1");
> -}
>  
> -function test1C() {
> +      let actual = content.document.getElementById("mctestdiv").innerHTML;

What's the point of checking this again after the waitForCondition call?

Also, shall we increase the scope of the expected string variable and reuse it (the same string is used again a few times)?

::: browser/base/content/test/siteIdentity/browser_bug902156.js:67
(Diff revision 2)
> -function test1D() {
> -  // The Control Center button should appear but isMixedContentBlocked should be NOT true,
> +    // The Control Center button should appear but isMixedContentBlocked should be NOT true,
> -  // because our decision of disabling the mixed content blocker is persistent.
> +    // because our decision of disabling the mixed content blocker is persistent.
> -  assertMixedContentBlockingState(gTestBrowser, {activeLoaded: true, activeBlocked: false, passiveLoaded: false});
> -
> -  var actual = content.document.getElementById("mctestdiv").innerHTML;
> +    assertMixedContentBlockingState(browser, {activeLoaded: true, activeBlocked: false, passiveLoaded: false});
> +    await ContentTask.spawn(browser, null, function() {
> +      let actual = content.document.getElementById("mctestdiv").innerHTML;

I'm not sure I fully understand the exact order of events, but how come we don't need a waitForCondition here like above?
Attachment #8894495 - Flags: review?(nhnt11) → review+
Comment on attachment 8894495 [details]
Bug 1368143 - Modernize browser_bug902156.js.

https://reviewboard.mozilla.org/r/165664/#review172090

> What's the point of checking this again after the waitForCondition call?
> 
> Also, shall we increase the scope of the expected string variable and reuse it (the same string is used again a few times)?

It was done originally so I kept it, I guess it's a style question.

> Also, shall we increase the scope of the expected string variable and reuse it (the same string is used again a few times)?

I think that's difficult to do because most of the assertions are inside ContentTasks.

> I'm not sure I fully understand the exact order of events, but how come we don't need a waitForCondition here like above?

Uhm, good question. I just transformed the original test, not sure what they were thinking. I think either way works so I'd prefer to keep it like that and not risk breaking anything.
I had forgotten to do a try push for this, doing it now.
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 442f9e522c80 -d 0a543fedee9c: rebasing 414214:442f9e522c80 "Bug 1368143 - Modernize browser_bug902156.js. r=nhnt11" (tip)
merging browser/base/content/test/siteIdentity/browser_bug902156.js
warning: conflicts while merging browser/base/content/test/siteIdentity/browser_bug902156.js! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Pushed by jhofmann@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b850c8c06d17
Modernize browser_bug902156.js. r=nhnt11
https://hg.mozilla.org/mozilla-central/rev/b850c8c06d17
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Whiteboard: [stockwell needswork] → [stockwell fixed:other]
You need to log in before you can comment on or make changes to this bug.