[iOS][PRDBD] EG tests for password removal from QuickDeleteBrowsingDVC [chromium/src : main]

0 views

Skip to first unread message

Angela Novakovic (Gerrit)

unread,

Jan 19, 2026, 10:00:35 AM (2 days ago) Jan 19

to Chromium LUCI CQ, Noémie St-Onge, chromium...@chromium.org, dullweb...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org, msrame...@chromium.org

Attention needed from Noémie St-Onge

Angela Novakovic added 11 comments

Commit Message

Line 7, Patchset 8:[iOS][PRDBD] Adds EG tests for the removal of the password cell.

Noémie St-Onge . resolved

It would be great if we could squeeze in some context over the concerned view in the title. We could have something like
```suggestion
[iOS][PRDBD] EG tests for password removal from QuickDeleteBrowsingDVC
```
I find, "Adds EG tests for the removal of the password cell" a bit too broad

Angela Novakovic

Done

File ios/chrome/browser/settings/ui_bundled/clear_browsing_data/quick_delete_browsing_data/test/quick_delete_browsing_data_egtest.mm

Line 305, Patchset 8:- (void)testCancelButtonDoesNotUpdatePrefs {

Noémie St-Onge . resolved

Why are we forcing the flag on for this test?

Angela Novakovic

Done

Line 331, Patchset 8:

  if (![QuickDeleteAppInterface
          isPasswordRemovalFromDeleteBrowsingDataEnabled]) {
    [[EarlGrey selectElementWithMatcher:ClearSavedPasswordsButton()]
        assertWithMatcher:elementIsSelectedMatcher(false)];
  }

Noémie St-Onge . resolved

How about we do:
```
  [[EarlGrey selectElementWithMatcher:ClearSavedPasswordsButton()]
      assertWithMatcher:[QuickDeleteAppInterface
                            isPasswordRemovalFromDeleteBrowsingDataEnabled]
                            ? grey_nil()
                            : elementIsSelectedMatcher(false)];
```

we could to the same below when asserting that all rows are selected

Angela Novakovic

Done

Line 401, Patchset 8:

    GREYAssertEqual(
        [ChromeEarlGrey userBooleanPref:browsing_data::prefs::kDeletePasswords],
        NO, @"Passwords pref changed on cancel.");

Noémie St-Onge . resolved

I don't think checking this part would hurt when the feature is enabled. If anything, it would just confirm that we're not inadvertently changing this pref

Angela Novakovic

Done

Line 426, Patchset 8:

  if (![QuickDeleteAppInterface
          isPasswordRemovalFromDeleteBrowsingDataEnabled]) {
    [ChromeEarlGrey setBoolValue:NO
                     forUserPref:browsing_data::prefs::kDeletePasswords];
  }

Noémie St-Onge . resolved

I think it's also fine to leave this part in when the feature is enabled. It shouldn't make the test fail
Same for all other instances in both EG test files

Angela Novakovic

Done

Line 447, Patchset 8:

  if (![QuickDeleteAppInterface
          isPasswordRemovalFromDeleteBrowsingDataEnabled]) {
    NoDeleteBrowsingDataDialogHistogram(
        DeleteBrowsingDataDialogAction::kPasswordsToggledOn);
  }

Noémie St-Onge . resolved

Same, the metric should be empty regardless of the feature's enabled state

Angela Novakovic

Done

Line 464, Patchset 8:

  if (![QuickDeleteAppInterface
          isPasswordRemovalFromDeleteBrowsingDataEnabled]) {
    [[EarlGrey selectElementWithMatcher:ClearSavedPasswordsButton()]
        assertWithMatcher:elementIsSelectedMatcher(false)];
  }

Noémie St-Onge . resolved

```
  [[EarlGrey selectElementWithMatcher:ClearSavedPasswordsButton()]
      assertWithMatcher:[QuickDeleteAppInterface
                            isPasswordRemovalFromDeleteBrowsingDataEnabled]
                            ? grey_nil()
                            : elementIsSelectedMatcher(false)];
```

Angela Novakovic

Done

Line 498, Patchset 8:

  if (![QuickDeleteAppInterface
          isPasswordRemovalFromDeleteBrowsingDataEnabled]) {
    [[EarlGrey selectElementWithMatcher:ClearSavedPasswordsButton()]
        assertWithMatcher:elementIsSelectedMatcher(true)];
  }

Noémie St-Onge . resolved

```
  [[EarlGrey selectElementWithMatcher:ClearSavedPasswordsButton()]
      assertWithMatcher:[QuickDeleteAppInterface
                            isPasswordRemovalFromDeleteBrowsingDataEnabled]
                            ? grey_nil()
                            : elementIsSelectedMatcher(true)];
```

Angela Novakovic

Done

Line 531, Patchset 8:

  if (![QuickDeleteAppInterface
          isPasswordRemovalFromDeleteBrowsingDataEnabled]) {
    GREYAssertEqual(
        [ChromeEarlGrey userBooleanPref:browsing_data::prefs::kDeletePasswords],
        YES, @"Failed to save passwords pref change on confirm.");
  }

Noémie St-Onge . resolved

```
  BOOL shouldPrefBeUpdated =
      ![QuickDeleteAppInterface isPasswordRemovalFromDeleteBrowsingDataEnabled];
  GREYAssertEqual(
      [ChromeEarlGrey userBooleanPref:browsing_data::prefs::kDeletePasswords],
      shouldPrefBeUpdated, @"Failed to save passwords pref change on confirm.");
```

Angela Novakovic

Done

Line 550, Patchset 8:

  if (![QuickDeleteAppInterface
          isPasswordRemovalFromDeleteBrowsingDataEnabled]) {
    ExpectDeleteBrowsingDataDialogHistogram(
        DeleteBrowsingDataDialogAction::kPasswordsToggledOn);
  }

Noémie St-Onge . resolved

```
  if ([QuickDeleteAppInterface
          isPasswordRemovalFromDeleteBrowsingDataEnabled]) {
    NoDeleteBrowsingDataDialogHistogram(
        DeleteBrowsingDataDialogAction::kPasswordsToggledOn);
  } else {
    ExpectDeleteBrowsingDataDialogHistogram(
        DeleteBrowsingDataDialogAction::kPasswordsToggledOn);
  }

```

Angela Novakovic

Done

File ios/chrome/browser/settings/ui_bundled/clear_browsing_data/test/quick_delete_egtest.mm

Line 291, Patchset 8:

  if ([self isRunningTest:@selector
            (testButtonColorWhenThePasswordRemovalFeatureIsDisabled)]) {
    config.features_disabled.push_back(kPasswordRemovalFromDeleteBrowsingData);
  }
  if ([self isRunningTest:@selector(testPasswordsForDeletion)]) {
    config.features_disabled.push_back(kPasswordRemovalFromDeleteBrowsingData);
  }
  if ([self isRunningTest:@selector(testKeepPasswords)]) {
    config.features_disabled.push_back(kPasswordRemovalFromDeleteBrowsingData);
  }

Noémie St-Onge . resolved

Please combine these into a single if statement

Angela Novakovic

Done

Open in Gerrit

Related details

Attention is currently required from:

Noémie St-Onge

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Noémie St-Onge (Gerrit)

unread,

Jan 19, 2026, 2:12:57 PM (2 days ago) Jan 19

to Angela Novakovic, Chromium LUCI CQ, chromium...@chromium.org, dullweb...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org, msrame...@chromium.org

Attention needed from Angela Novakovic

Noémie St-Onge added 4 comments

File ios/chrome/browser/settings/ui_bundled/clear_browsing_data/quick_delete_browsing_data/test/quick_delete_browsing_data_egtest.mm

Line 159, Patchset 11 (Latest): // These tests will always run with the feature

Noémie St-Onge . unresolved

nit: `This test`

Line 426, Patchset 8:

  if (![QuickDeleteAppInterface
          isPasswordRemovalFromDeleteBrowsingDataEnabled]) {
    [ChromeEarlGrey setBoolValue:NO
                     forUserPref:browsing_data::prefs::kDeletePasswords];
  }

Noémie St-Onge . unresolved

I think it's also fine to leave this part in when the feature is enabled. It shouldn't make the test fail
Same for all other instances in both EG test files

Angela Novakovic

Done

Noémie St-Onge

There are a couple instances left in both test files

Line 521, Patchset 11 (Latest): BOOL shouldPrefBeUpdated =

Noémie St-Onge . unresolved

Sorry, should have been more precise when I made the initial suggestion
```suggestion
  BOOL shouldPasswordPrefBeUpdated =
```

File ios/chrome/browser/settings/ui_bundled/clear_browsing_data/test/quick_delete_egtest.mm

Line 291, Patchset 11 (Latest):

      [self isRunningTest:@selector
            (testButtonColorWhenThePasswordRemovalFeatureIsDisabled)] ||
      [self isRunningTest:@selector(testPasswordsForDeletion)] ||
      [self isRunningTest:@selector(testKeepPasswords)]) {

Noémie St-Onge . unresolved

Shouldn't the flag be `disabled` for these tests?

Open in Gerrit

Related details

Attention is currently required from:

Angela Novakovic

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review

No-Unresolved-Comments

Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Angela Novakovic (Gerrit)

unread,

9:22 AM (4 hours ago) 9:22 AM

to Chromium LUCI CQ, Noémie St-Onge, chromium...@chromium.org, dullweb...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org, msrame...@chromium.org

Attention needed from Noémie St-Onge

Angela Novakovic added 3 comments

File ios/chrome/browser/settings/ui_bundled/clear_browsing_data/quick_delete_browsing_data/test/quick_delete_browsing_data_egtest.mm

Line 159, Patchset 11: // These tests will always run with the feature

Noémie St-Onge . resolved

nit: `This test`

Angela Novakovic

Done

Line 521, Patchset 11: BOOL shouldPrefBeUpdated =

Noémie St-Onge . resolved

Sorry, should have been more precise when I made the initial suggestion
```suggestion
  BOOL shouldPasswordPrefBeUpdated =
```

Angela Novakovic

Done

File ios/chrome/browser/settings/ui_bundled/clear_browsing_data/test/quick_delete_egtest.mm

Line 291, Patchset 11: [self isRunningTest:@selector


            (testButtonColorWhenThePasswordRemovalFeatureIsDisabled)] ||
      [self isRunningTest:@selector(testPasswordsForDeletion)] ||
      [self isRunningTest:@selector(testKeepPasswords)]) {

Noémie St-Onge . resolved

Shouldn't the flag be `disabled` for these tests?

Angela Novakovic

Done

Open in Gerrit

Related details

Attention is currently required from:

Noémie St-Onge

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Reply all

Reply to author

Forward

0 new messages