Convert test_bug260264.html to use SpecialPowers

RESOLVED FIXED in mozilla33

Status

Testing
Mochitest
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: Martijn Wargers (zombie), Assigned: Martijn Wargers (zombie))

Tracking

unspecified
mozilla33
x86
Mac OS X
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 5 obsolete attachments)

(Assignee)

Description

5 years ago
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.
(Assignee)

Updated

5 years ago
QA Contact: martijn.martijn
(Assignee)

Updated

5 years ago
Assignee: nobody → martijn.martijn
(Assignee)

Comment 2

5 years ago
Created attachment 805636 [details] [diff] [review]
873403.diff

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.
(Assignee)

Comment 3

5 years ago
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)
(Assignee)

Comment 4

5 years ago
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-
(Assignee)

Comment 6

5 years ago
(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 :/
(Assignee)

Comment 7

5 years ago
Created attachment 806330 [details] [diff] [review]
873403.diff

Ok, this should have the amount of pushPrefEnv/pushPermissions limited to a minimum.
Attachment #805636 - Attachment is obsolete: true
Attachment #806330 - Flags: review?(jmaher)
(Assignee)

Comment 8

5 years ago
Created attachment 806582 [details]
test_bug260264_nested.html (wip)

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+
(Assignee)

Comment 10

5 years ago
Created attachment 806985 [details] [diff] [review]
873403.diff

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+
(Assignee)

Comment 12

5 years ago
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)
(Assignee)

Comment 13

5 years ago
Created attachment 807682 [details] [diff] [review]
873403.diff (git version)
Attachment #806985 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #806330 - Attachment is obsolete: true
(Assignee)

Comment 15

5 years ago
Tryserver didn't have any problems with the patch.
(Assignee)

Comment 16

5 years ago
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)
(Assignee)

Comment 19

5 years ago
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)
(Assignee)

Comment 21

4 years ago
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)
(Assignee)

Comment 24

4 years ago
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.
(Assignee)

Comment 25

4 years ago
Created attachment 8455223 [details] [diff] [review]
873403.diff (for check-in)
Attachment #807682 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/484e2c6bd619
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.