Closed Bug 1506390 Opened 4 years ago Closed 4 years ago

ensure the default permissions do not intefer with the permission tests that assume no defaults

Categories

(Core :: Permission Manager, defect, P3)

Product:
Core
Component:
Permission Manager
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla65
Tracking Flags:
Tracking Status
firefox-esr60 --- fixed
firefox63 --- wontfix
firefox64 --- fixed
firefox65 --- fixed

People

(Reporter: mkmelin, Assigned: mkmelin)

References

Details

Attachments

(1 file)

bug1506390_permmmanager_tests.patch
8.91 KB, patch
johannh
: review+
Details | Diff | Splinter Review 
As I found out in bug 1072748, the xpcshell test do not run with the Firefox preferences (bug 1168178), so for the permission manager tests they implicitly assume no defaults.

This bug is to ensure these tests explicitly ensure there are no default prefs involved. (Thunderbird xpcshell tests do run with the real prefs, and therefore get test failures due to the assumptions).

        Attachment #9024208 -
        Flags: review?(ehsan)

    
  
Blocks: 1072748

    
    

    
  
Comment on attachment 9024208 [details] [diff] [review]
bug1506390_permmmanager_tests.patch

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

Please reset these prefs at the end of the test or use SpecialPowers.pushPrefEnv which I think should also be available in xpcshell.
Priority: -- → P3

    
    

    
  
(In reply to Johann Hofmann [:johannh] from comment #2)
> Please reset these prefs at the end of the test or use

Seems like wasted effort since each xpcshell tests get a clean environment when the start?
https://searchfox.org/mozilla-central/rev/7f7c353e969e61a6a85201cc8ad3c3de12ac30d8/testing/xpcshell/runxpcshelltests.py#803

Since you looked, maybe you want to review?
Flags: needinfo?(jhofmann)

    
    

    
  
Comment on attachment 9024208 [details] [diff] [review]
bug1506390_permmmanager_tests.patch

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

Ah, that's run per individual test? In any case, since xpcshell tests aren't run with that pref anyway, this seems like a safe thing to do. Stealing review from Ehsan...

        Attachment #9024208 -
        Flags: review?(ehsan) → review+
Flags: needinfo?(jhofmann)

    
    

    
  
Yes, for each test. I think re-tries would be very messy otherwise.

    
    

    
  
Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ca52fc256662
ensure the default permissions do not intefer with the permission tests that assume no defaults. r=johannh

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/ca52fc256662
Status: ASSIGNED → RESOLVED
Closed: 4 years ago

          status-firefox65:
          --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla65

    
    

    
  
Comment on attachment 9024208 [details] [diff] [review]
bug1506390_permmmanager_tests.patch

This is "test only". Can we please get this uplifted? Sorry, I forgot what the procedure is here.
Flags: needinfo?(ryanvm)

        Attachment #9024208 -
        Flags: approval-mozilla-esr60?

        Attachment #9024208 -
        Flags: approval-mozilla-beta?

        Attachment #9024208 -
        Flags: approval-mozilla-esr60?

        Attachment #9024208 -
        Flags: approval-mozilla-beta?

          status-firefox63:
          --- → wontfix

          status-firefox64:
          --- → fix-optional

          status-firefox-esr60:
          --- → fix-optional
Whiteboard: [checkin-needed-beta][checkin-needed-esr60]
Flags: needinfo?(ryanvm)

    
    

    
  
https://hg.mozilla.org/releases/mozilla-beta/rev/a96813612e0c

          status-firefox64:
          fix-optional → fixed
Whiteboard: [checkin-needed-beta][checkin-needed-esr60]

    
    

    
  
https://hg.mozilla.org/releases/mozilla-esr60/rev/2d3326755541b844e6c193c3e839d4fb91236444 on THUNDERBIRD_60_VERBRANCH
Thanks Ryan, I put this on our branch since we want to try it before 60.4 comes around.

          You need to log in
          before you can comment on or make changes to this bug.