Closed Bug 873403 Opened 7 years ago Closed 6 years ago

Convert test_bug260264.html to use SpecialPowers

Categories

(Testing :: Mochitest, defect)

x86
macOS
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla33

People

(Reporter: martijn.martijn, Assigned: martijn.martijn)

References

()

Details

Attachments

(1 file, 5 obsolete files)

I split this of from bug 868438.

test_bug260264.html has 61 tests in it and I see the popup blocker appearing with 19 reported blocked popups.

This test needs to be rewritten in order for it to work in b2g mochitest.

I have wip patch in bug 868438 (which is probably incorrect, so I should just rewrite it).

Note that bug 859401 needs to be checked in, because this fix will depend on some SpecialPowers api change (for pushPermissions/addPermission) that is needed for this.
QA Contact: martijn.martijn
Assignee: nobody → martijn.martijn
Attached patch 873403.diff (obsolete) — Splinter Review
Ok, I managed to rewrite test_bug260264.html to use pushPermissions/pushPrefEnv.
But there is also test_bug260264_nested.html which needs to be rewritten.
Comment on attachment 805636 [details] [diff] [review]
873403.diff

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

This test file can't be enabled on b2g mochitest, because the b2g mochitest app doesn't have popup blocking implemented (yet?).
But this is still a good rewrite to pushPrefEnv/pushPermissions, which should be the way to write tests.
Attachment #805636 - Flags: review?(jmaher)
test_bug260264_nested.html can't be run as a standalone test, btw.
Comment on attachment 805636 [details] [diff] [review]
873403.diff

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

please push back if there is not a better way to do all of this.

::: dom/tests/mochitest/bugs/test_bug260264.html
@@ +29,5 @@
>      checkOpened = function() { ok(window.open("http://example.com"), "not properly opened") },
>      checkBlocked = function() { ok(!window.open("http://example.com"), "not properly blocked") };
>  
> +function run_tests() {
> +  SpecialPowers.pushPrefEnv({"set": [["dom.popup_allowed_events", "click mouseup"]]}, run_tests2);

all we do is pushprefenv here, test_sanity2 does the same thing, can we combine these prefs?

@@ +39,4 @@
>  }
>  
> +function run_tests3() {
> +  SpecialPowers.pushPrefEnv({"set": [["dom.popup_maximum", 3]]}, run_tests4);

why is run_tests2 a pushPermissions and this is just another pref?  Can we combine the prefs from run_tests, test_sanity and this to be what we need?

@@ +76,5 @@
> +  SpecialPowers.pushPermissions([{'type': 'popup', 'allow': ALLOW_ACTION, 'context': document}], run_tests7);
> +}
> +
> +function run_tests7() {
> +  SpecialPowers.pushPrefEnv({"set": [["dom.popup_maximum", 3]]}, run_tests8);

I really wish we could pushPermission and pushPrefEnv together?  This is probably the single test case that uses so much of this, maybe:
SpecialPowers.pushPermissions([{'type': 'popup', 'allow': ALLOW_ACTION, 'context': document}], function() {SpecialPowers.pushPrefEnv({"set": [["dom.popup_maximum", 3]]}, run_tests8); });

that gets sort of hard to read/edit.

@@ +114,5 @@
> +  SpecialPowers.pushPermissions([{'type': 'popup', 'allow': DENY_ACTION, 'context': document}], run_tests10);
> +}
> +
> +function run_tests10() {
> +  SpecialPowers.pushPrefEnv({"set": [["dom.popup_maximum", 3]]}, run_tests11);

why run_tests8 does pushPrefEnv, then run_tests9 does only a pushPermissions, then we do a pushPrefEnv here
Attachment #805636 - Flags: review?(jmaher) → review-
(In reply to Joel Maher (:jmaher) from comment #5)
> please push back if there is not a better way to do all of this.

No,you're right. It was quite difficult to convert this test file to use pushPrefEnv/pushPermissions, so this is still the version that is mostly a 1-on-1 conversion. It can be optimized considerably, I'll do that.

test_bug260264_nested.html looks even more complicated :/
Attached patch 873403.diff (obsolete) — Splinter Review
Ok, this should have the amount of pushPrefEnv/pushPermissions limited to a minimum.
Attachment #805636 - Attachment is obsolete: true
Attachment #806330 - Flags: review?(jmaher)
Attached file test_bug260264_nested.html (wip) (obsolete) —
Ok, I think I might be able to convert test_bug260264_nested.html. It turns out that the _alter_helper gets confused by all the cruft that gets added when you run it as a standalone test. I'll replacing that with something simpler.
Comment on attachment 806330 [details] [diff] [review]
873403.diff

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

much better.  While this isn't easy to read and understand, it wasn't with the original case and now it is at least simplified greatly from the previous patch.
Attachment #806330 - Flags: review?(jmaher) → review+
Attached patch 873403.diff (obsolete) — Splinter Review
Ok, I managed to rewrite test_bug260264_nested.html to use SpecialPowers/pushPrefEnv.
I didn't change anything to test_bug260264_nested.html.
Attachment #806582 - Attachment is obsolete: true
Attachment #806985 - Flags: review?(jmaher)
Comment on attachment 806985 [details] [diff] [review]
873403.diff

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

to be honest I am not able to follow this.  logically with prefs and permissions and specialpowers this appears to be a sane set of code, but the logic and what it is testing is out of my scope.  Please ensure that somebody who knows more about this test case can comment on it (specifically the nested stuff)
Attachment #806985 - Flags: review?(jmaher) → review+
It is indeed difficult to follow. Afaict, I didn't change what the test files are testing. I followed what the original code was doing.
Ben Newman (he doesn't seem to be active in bugzilla anymore) or Johnny Stenback (who reviewed these tests before), could you comment on whether I rewrote these tests correctly?
Flags: needinfo?(mozilla+ben)
Attached patch 873403.diff (git version) (obsolete) — Splinter Review
Attachment #806985 - Attachment is obsolete: true
Attachment #806330 - Attachment is obsolete: true
Tryserver didn't have any problems with the patch.
Is it ok to land this? Is there somebody else, who would be able to know if this rewrite to SpecialPowers for these tests is all right?
This basically needs careful review from someone who will verify that none of the behavior got changed, right?  This stuff is _very_ fiddly.  It doesn't sound like this careful review happened so far...  :(
Seems like it's been difficult to find the right reviewer. Martijn mentioned Johnny Stenback above, so requesting more info from him.
Flags: needinfo?(jst)
Comment on attachment 807682 [details] [diff] [review]
873403.diff (git version)

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

I talked with Johnny in person, and he said he already looked at it a little bit and he will be going to look at it further. Thanks Johnny!
Attachment #807682 - Flags: review?(jst)
Clearing unnecessary needinfo requests, this is in my review queue, sorry it's taking so long to get to :(
Flags: needinfo?(mozilla+ben)
Flags: needinfo?(jst)
A new year has come and I'm sure it's full of good intentions from you, Johnny ;)
Flags: needinfo?(jst)
Comment on attachment 807682 [details] [diff] [review]
873403.diff (git version)

Indeed :) Sorry this took so long, this required me to find a good chunk of time to sit down and stare at before/after this patch and trace through and make sure that this doesn't actually change any behavior here, and AFAICT it does not. r=jst
Attachment #807682 - Flags: review?(jst) → review+
Oh, and I meant to say that it does seem like we could retain the original functions in the main test here (i.e. testIntentional, testProbablyIntentional, and maybe even testProbablyUnintentional) to avoid having to duplicate the sequence of operations that can run one after the other even with SpecialPowers, but I'm not going to block this landing on that happening. If someone wants to take that on to make this a bit easier of a read, I'd gladly review that change as well.
Flags: needinfo?(jst)
Thanks for the review! Yeah, probably those original functions could be retained. I'm not sure it would be easier to read, though. When an unexpected fail happens, it can be easier to find what is going on, when not everyting is wrapped up in helper functions.
Attachment #807682 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/484e2c6bd619
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.