Closed Bug 1048098 Opened 5 years ago Closed 5 years ago

Remove SpecialPowers.flushPermissions, flushPrefEnv from tearing down procedure

Categories

(Firefox OS Graveyard :: RIL, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
2.1 S2 (15aug)

People

(Reporter: vicamo, Assigned: vicamo)

References

Details

(Whiteboard: [p=1])

Attachments

(1 file, 3 obsolete files)

"testing/specialpowers/content/specialpowersAPI.js" has specified that permissions pushed by pushPermissions() and preference settings pushed by pushPrefEnv() will be reverted when the test finishes.  Therefore there is no reason to include any of them in test case tearing down procedure.

Besides, I observed some failures due to timeouts in waiting for flushPermissions/flushPrefEnv returns.

Known affected are:

dom/mobilemessage/tests/marionette/head.js:571:    SpecialPowers.flushPermissions(function() {
dom/mobilemessage/tests/marionette/head.js:574:      SpecialPowers.flushPrefEnv(function() {
dom/mobileconnection/tests/marionette/head.js:1135:    SpecialPowers.flushPermissions(function() {
dom/bluetooth/tests/marionette/head.js:712:    SpecialPowers.flushPermissions(function() {
dom/bluetooth2/tests/marionette/head.js:633:    SpecialPowers.flushPermissions(function() {
dom/cellbroadcast/tests/marionette/head.js:324:    SpecialPowers.flushPermissions(function() {
Blocks: 916607
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → vyang
Attachment #8466868 - Flags: review?(echen)
Attachment #8466868 - Flags: review?(btian)
Sorry, wrong file attached.
Attachment #8466868 - Attachment is obsolete: true
Attachment #8466868 - Flags: review?(echen)
Attachment #8466868 - Flags: review?(btian)
Attachment #8466869 - Flags: review?(echen)
Attachment #8466869 - Flags: review?(btian)
No longer blocks: 916607
Blocks: 1047767
Duplicate of this bug: 1047767
Comment on attachment 8466869 [details] [diff] [review]
bug-1048098-remove-redundant-flushPermissions-flushPrefEnv.patch

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

r=me on the cellbroadcast/mobileconnection/mobilemessage changes.

::: dom/cellbroadcast/tests/marionette/head.js
@@ +316,5 @@
>    return Promise.all(promises).then(aResults => aResults[0].message);
>  }
>  
>  /**
>   * Flush permission settings and call |finish()|.

nit: Since we don't flush permission any more, could you help to revise the comments? Thank you.

::: dom/mobileconnection/tests/marionette/head.js
@@ +1127,5 @@
>    return _numOfRadioInterfaces;
>  }
>  
>  /**
>   * Flush permission settings and call |finish()|.

Ditto

::: dom/mobilemessage/tests/marionette/head.js
@@ +561,4 @@
>  }
>  
>  /**
>   * Flush permission settings and call |finish()|.

Ditto
Attachment #8466869 - Flags: review?(echen) → review+
(In reply to Edgar Chen [:edgar][:echen] from comment #4)
> nit: Since we don't flush permission any more, could you help to revise the
> comments? Thank you.

Ah, yes. Didn't notice them.
Comment on attachment 8466869 [details] [diff] [review]
bug-1048098-remove-redundant-flushPermissions-flushPrefEnv.patch

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

r=me with comment addressed. Thanks.

::: dom/bluetooth/tests/marionette/head.js
@@ +704,5 @@
>    return deferred.promise;
>  }
>  
>  /**
>   * Flush permission settings and call |finish()|.

Please revise comment as other files.

::: dom/bluetooth2/tests/marionette/head.js
@@ +625,5 @@
>    return true;
>  }
>  
>  /**
>   * Flush permission settings and call |finish()|.

Ditto.
Attachment #8466869 - Flags: review?(btian) → review+
Attached patch patch : v2 (obsolete) — Splinter Review
Correct function documentation to reflect the changes. Address review comment 4 and comment 6.
Attachment #8466869 - Attachment is obsolete: true
Attachment #8467609 - Flags: review+
Whiteboard: [p=1]
Target Milestone: --- → 2.1 S2 (15aug)
One more flushPermissions in dom/events/test, but that should not be a problem anyway. Included in https://tbpl.mozilla.org/?tree=Try&rev=ed773e1fbdea
Attached patch patch : v3Splinter Review
Include patch to dom/events/test .
Attachment #8467609 - Attachment is obsolete: true
Attachment #8468303 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/95aa9681d4f8
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.