NotificationsTest.TestOriginPrefsNotSavedInIncognito should not fail |
|||||||
Issue descriptionThis interactive ui test was disabled with a comment linking it to a crash which was fixed in 2012 ( Issue 160657 ), so it's probably time to re-enable it. https://cs.chromium.org/chromium/src/chrome/browser/notifications/notification_interactive_uitest.cc?l=365 However I just tried un-disabling & running it on Linux with the following command and it now fails: ninja -C out/LinuxDebug -j2000 -l20 interactive_ui_tests && out/LinuxDebug/interactive_ui_tests --gtest_filter="*NotificationsTest.TestOriginPrefsNotSaved*" --extract-test-list-from-filter ../../chrome/browser/notifications/notification_interactive_uitest.cc:377: Failure Value of: RequestAndAcceptPermission(incognito) Actual: false Expected: true
,
May 14 2018
Seems the crash in #1 occurs reliably on one of my monitors but not the other. Weird.
,
May 14 2018
The crash in #c3 is unrelated to the failure here. Reading the code, the ASSERT on notification_interactive_uitest.cc:378 is verifying that permission was *granted* in an Incognito context. We never grant permission in an Incognito context. Is the test just void?
,
May 14 2018
Agree this test should not be testing that permission can be granted in incognito. However we should have *something* testing that the notification auto-denial is not persisted after the incognito session. NotificationPermissionContextTest.TestDenyInIncognitoAfterDelay currently tests that the autoblock occurs but not that this block is not persisted. Maybe we can add another line there to do so? https://cs.chromium.org/chromium/src/chrome/browser/notifications/notification_permission_context_unittest.cc?l=281
,
May 14 2018
Peter pointed out that the general permission/content-settings tests should have us covered here, since *all* incognito settings should only be stored in memory for the duration of the incognito session, not just notification settings. So this specialized legacy test can be deleted. However after a cursory search I could not find any test for incognito settings not being persisted after the incognito session has closed. But maybe I'm missing something?
,
May 16 2018
Can anyone from the Privacy team comment on #5? ie. 1. Is it indeed true to say '*all* incognito settings should only be stored in memory for the duration of the incognito session and no longer'? 2. Do we have adequate test coverage for this?
,
May 16 2018
Yes, *all* incognito settings are temporary and should note be written to disk, but I am not sure about the test coverage. My plan in this quarter and the next one is to check for it and make sure it is complete. So we can assign this one to me as well, and based on the progress of things we will decide about it.
,
May 16 2018
Good plan - thanks. Have created Issue 843494 for ensuring test coverage, assigned to you. I think the outcome of NotificationsTest.TestOriginPrefsNotSavedInIncognito should be that it's deleted in favour of non-content-setting-specific tests given the rule that incognito settings should not be persisted is not specific to notification settings. Will keep this bug open & assigned to me for removing that test as such.
,
May 16 2018
,
May 16 2018
I agree. OK.
,
May 21 2018
Dear awdf, This review has now been inactive for over 3 days. Could you please take appropriate steps to finish the review? Your friendly privacy review bot.
,
May 22 2018
,
May 29 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/7caf3a2eeb70cecaa0574fac1fa8f7569013982f commit 7caf3a2eeb70cecaa0574fac1fa8f7569013982f Author: Anita Woodruff <awdf@chromium.org> Date: Tue May 29 16:18:57 2018 [Tests] Delete NotificationsTest.TestOriginPrefsNotSavedInIncognito - This test was disabled long ago due to a crash, which no longer occurs, however, the test is now out of date, and fails for unknown reasons. - Rather than attempt to fix up only this test for the incognito persistence behaviour of notification content settings specifically, it would be more appropriate to test the incognito persistence behaviour of *all* content settings from content settings tests, since that's where the logic lives. Filed https://crbug.com/843494 to ensure this is tested more generally. Bug: 841203 Change-Id: I40ef6e73af449756c93e1afe8001c0dd9c9af1c2 Reviewed-on: https://chromium-review.googlesource.com/1065995 Commit-Queue: Anita Woodruff <awdf@chromium.org> Reviewed-by: Peter Beverloo <peter@chromium.org> Cr-Commit-Position: refs/heads/master@{#562451} [modify] https://crrev.com/7caf3a2eeb70cecaa0574fac1fa8f7569013982f/chrome/browser/notifications/notification_interactive_uitest.cc
,
May 30 2018
|
|||||||
►
Sign in to add a comment |
|||||||
Comment 1 by awdf@chromium.org
, May 14 2018