[iOS][PRDBD] Adds unit tests for the removal of the password option [chromium/src : main]
Angela Novakovic (Gerrit)
Angela Novakovic voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Noémie St-Onge
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Noémie St-Onge (Gerrit)
Noémie St-Onge added 17 comments![]( "Open in Gerrit")
selected && !IsPasswordRemovalFromDeleteBrowsingDataEnabled()No need for this as we would have crashed on line 486 already
base::test::ScopedFeatureList feature_list_;Since a lot of the test cases don't need the ScopedFeatureList, we can instead define it at the test case level when needed. Doing so will also lower the chances of forgetting to remove this variable after the feature launched. The resulting number of lines should also be pretty similar
To enable the flag:
```
base::test::ScopedFeatureList feature_list(kPasswordRemovalFromDeleteBrowsingData);
```
To disable the flag:
```
base::test::ScopedFeatureList feature_list;
feature_list.InitAndDisableFeature(kPasswordRemovalFromDeleteBrowsingData);
```
if (IsPasswordRemovalFromDeleteBrowsingDataEnabled()) {
GTEST_SKIP() << "The test is skipped as the feature flag "
"`kPasswordRemovalFromDeleteBrowsingData` is enabled, thus "
"the password summary is not available.";
return;
}We should prefer forcing the flag off using the `ScopedFeatureList` as it will ensure a full coverage. Given the simplicity and lightness of unit tests, we should aim at running every test case and not depend on the current state of the feature flag
Also applies to the other skipped test cases
// teh feature flag `kPasswordRemovalFromDeleteBrowsingData` is disabled.typo: `teh -> the`
GTEST_SKIP()
<< "The test is skipped as the feature flag "
"`kPasswordRemovalFromDeleteBrowsingData` is enabled, thus the test "
"cannot be done using passwords as a type of data.";I think this test also applies when the feature is enabled. I understand that it will fail if it stays as is because we are using the password data type, but I think we could simply work around that by replacing the passwords-related stuff with another data type. The goal of this test is to verify the summary with multiple data types, not to test passwords specifically
// teh feature flag `kPasswordRemovalFromDeleteBrowsingData` is enabled.typo: `teh -> the`
TEST_F(QuickDeleteMediatorTest,
TestSummaryWithSeveralTypesWithThePasswordRemovalFeatureEnabled) {See my comment for the test above. I don't think we need both
// Tests that setPasswordsSelection is not called when the feature`` `setPasswordsSelection` ``
// Regardless of the pref value, setPasswordsSelection will not be called by`` `setPasswordsSelection` ``
// The pref value of `kDeletePasswords` will not be updated when the feature
// flag `kPasswordRemovalFromDeleteBrowsingData` is enabled.
prefs()->SetBoolean(browsing_data::prefs::kDeletePasswords, true);
prefs()->SetBoolean(browsing_data::prefs::kCloseTabs, true);
OCMReject([consumer_ setPasswordsSelection:YES]);
OCMExpect([consumer_ setTabsSelection:YES]);Is this relevant to this specific test case? If not, I don't think we need to test it again
NSString* expectedSummary = [NSStringshould be in snake_case since this is C++
NSString* notExpectedSummary = [NSStringshould be in snake_case since this is C++, would also rename it to `unexpected_summary` or `wrong_summary`
OCMExpect([consumer_ setBrowsingDataSummary:expectedSummary]);
OCMReject([consumer_ setBrowsingDataSummary:notExpectedSummary]);FYI: If your goal is just to verify that the passed summary doesn't contain the string for passwords, you can use `OCMArg` to verify the argument:
```
OCMExpect([consumer_
setBrowsingDataSummary:[OCMArg checkWithBlock:^(NSString* summary) {
return ![summary
containsString:l10n_util::GetPluralNSStringF(
IDS_IOS_DELETE_BROWSING_DATA_SUMMARY_PASSWORDS,
1)];
}]]);
```
// Setting the consumers calls the createCounters method.`` `createCounters` ``
// TODO(crbug.com/463402932): Remove the below unit test after clean up.We don't really need a TODO here as we'll know that the entire test case should be removed when we'll clean up the flag disablement
// Setting the consumer calls the createCounters method.``` `createCounters` ``
- (NSUInteger)CounterCount;Should start with a lowercase letter as this is Objective-C -> `counterCount`
Related details
- Angela Novakovic
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. |
Angela Novakovic (Gerrit)
Angela Novakovic added 17 comments![]( "Open in Gerrit")
selected && !IsPasswordRemovalFromDeleteBrowsingDataEnabled()No need for this as we would have crashed on line 486 already
Done
Since a lot of the test cases don't need the ScopedFeatureList, we can instead define it at the test case level when needed. Doing so will also lower the chances of forgetting to remove this variable after the feature launched. The resulting number of lines should also be pretty similar
To enable the flag:
```
base::test::ScopedFeatureList feature_list(kPasswordRemovalFromDeleteBrowsingData);
```To disable the flag:
```
base::test::ScopedFeatureList feature_list;
feature_list.InitAndDisableFeature(kPasswordRemovalFromDeleteBrowsingData);
```
Done
if (IsPasswordRemovalFromDeleteBrowsingDataEnabled()) {
GTEST_SKIP() << "The test is skipped as the feature flag "
"`kPasswordRemovalFromDeleteBrowsingData` is enabled, thus "
"the password summary is not available.";
return;
}We should prefer forcing the flag off using the `ScopedFeatureList` as it will ensure a full coverage. Given the simplicity and lightness of unit tests, we should aim at running every test case and not depend on the current state of the feature flag
Also applies to the other skipped test cases
Done
// teh feature flag `kPasswordRemovalFromDeleteBrowsingData` is disabled.Angela Novakovictypo: `teh -> the`
Done
GTEST_SKIP()
<< "The test is skipped as the feature flag "
"`kPasswordRemovalFromDeleteBrowsingData` is enabled, thus the test "
"cannot be done using passwords as a type of data.";I think this test also applies when the feature is enabled. I understand that it will fail if it stays as is because we are using the password data type, but I think we could simply work around that by replacing the passwords-related stuff with another data type. The goal of this test is to verify the summary with multiple data types, not to test passwords specifically
Done
// teh feature flag `kPasswordRemovalFromDeleteBrowsingData` is enabled.Angela Novakovictypo: `teh -> the`
Done
TEST_F(QuickDeleteMediatorTest,
TestSummaryWithSeveralTypesWithThePasswordRemovalFeatureEnabled) {See my comment for the test above. I don't think we need both
Done
// Tests that setPasswordsSelection is not called when the featureAngela Novakovic`` `setPasswordsSelection` ``
Done
// Regardless of the pref value, setPasswordsSelection will not be called byAngela Novakovic`` `setPasswordsSelection` ``
Done
// The pref value of `kDeletePasswords` will not be updated when the feature
// flag `kPasswordRemovalFromDeleteBrowsingData` is enabled.
prefs()->SetBoolean(browsing_data::prefs::kDeletePasswords, true);
prefs()->SetBoolean(browsing_data::prefs::kCloseTabs, true);
OCMReject([consumer_ setPasswordsSelection:YES]);
OCMExpect([consumer_ setTabsSelection:YES]);Is this relevant to this specific test case? If not, I don't think we need to test it again
After looking at this test again, I think it's not possible to do it properly. The triggerUpdateUICallbackForPasswordsResults would have to be called and still result in a summary without passwords, but as it is using the FakeBrowsingDataCounterWrapperProducer, it is not disabled for passwords. I think we can skip this test. What do you think?
should be in snake_case since this is C++
Done
should be in snake_case since this is C++, would also rename it to `unexpected_summary` or `wrong_summary`
Done
OCMExpect([consumer_ setBrowsingDataSummary:expectedSummary]);
OCMReject([consumer_ setBrowsingDataSummary:notExpectedSummary]);FYI: If your goal is just to verify that the passed summary doesn't contain the string for passwords, you can use `OCMArg` to verify the argument:
```
OCMExpect([consumer_
setBrowsingDataSummary:[OCMArg checkWithBlock:^(NSString* summary) {
return ![summary
containsString:l10n_util::GetPluralNSStringF(
IDS_IOS_DELETE_BROWSING_DATA_SUMMARY_PASSWORDS,
1)];
}]]);
```
Done
// Setting the consumers calls the createCounters method.Angela Novakovic`` `createCounters` ``
Done
// TODO(crbug.com/463402932): Remove the below unit test after clean up.We don't really need a TODO here as we'll know that the entire test case should be removed when we'll clean up the flag disablement
Done
// Setting the consumer calls the createCounters method.Angela Novakovic``` `createCounters` ``
Done
Should start with a lowercase letter as this is Objective-C -> `counterCount`
- Noémie St-Onge
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. |
Angela Novakovic (Gerrit)
Angela Novakovic voted Commit-Queue+1![]( "Open in Gerrit")
- Angela Novakovic
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. |
Noémie St-Onge (Gerrit)
Noémie St-Onge added 5 comments![]( "Open in Gerrit")
TEST_F(QuickDeleteMediatorTest,This test is also valid when the feature is disabled, it just needs to be adapted, see comments below
// dispatches if all counters have returned.```suggestion
// dispatched if all counters have returned.
```
triggerUpdateUICallbackForAutofillResults(0, 0, 0);```suggestion
triggerUpdateUICallbackForAutofillResults(0, 0, 0);
if (!IsPasswordRemovalFromDeleteBrowsingDataEnabled()) {
triggerUpdateUICallbackForPasswordsResults(0);
}
```
SetConsumerWithThePasswordRemovalFeatureEnabled) {This test name doesn't really describe what's being tested here and could apply to the other tests below
EXPECT_EQ(4U, [fake_browsing_data_counter_wrapper_producer_ counterCount]);Would it be possible to instead have the `fake_browsing_data_counter_wrapper_producer_` return all the callbacks so that we can check the pref name associated with them and confirm that it is indeed the passwords one that's not present?
We could also return a vector with the pref names only
Related details
- Angela Novakovic
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. |
Angela Novakovic (Gerrit)
Angela Novakovic added 6 comments![]( "Open in Gerrit")
This test is also valid when the feature is disabled, it just needs to be adapted, see comments below
Done
```suggestion
// dispatched if all counters have returned.
```
Done
```suggestion
triggerUpdateUICallbackForAutofillResults(0, 0, 0);
if (!IsPasswordRemovalFromDeleteBrowsingDataEnabled()) {
triggerUpdateUICallbackForPasswordsResults(0);
}
```
Done
This test name doesn't really describe what's being tested here and could apply to the other tests below
Done
EXPECT_EQ(4U, [fake_browsing_data_counter_wrapper_producer_ counterCount]);Would it be possible to instead have the `fake_browsing_data_counter_wrapper_producer_` return all the callbacks so that we can check the pref name associated with them and confirm that it is indeed the passwords one that's not present?
We could also return a vector with the pref names only
Done
// The pref value of `kDeletePasswords` will not be updated when the feature
// flag `kPasswordRemovalFromDeleteBrowsingData` is enabled.
prefs()->SetBoolean(browsing_data::prefs::kDeletePasswords, true);
prefs()->SetBoolean(browsing_data::prefs::kCloseTabs, true);
OCMReject([consumer_ setPasswordsSelection:YES]);
OCMExpect([consumer_ setTabsSelection:YES]);Angela NovakovicIs this relevant to this specific test case? If not, I don't think we need to test it again
After looking at this test again, I think it's not possible to do it properly. The triggerUpdateUICallbackForPasswordsResults would have to be called and still result in a summary without passwords, but as it is using the FakeBrowsingDataCounterWrapperProducer, it is not disabled for passwords. I think we can skip this test. What do you think?
- Noémie St-Onge
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Noémie St-Onge (Gerrit)
Noémie St-Onge added 4 comments![]( "Open in Gerrit")
const std::set<std::string>& registeredPrefs =`registered_prefs` as this is C++
EXPECT_TRUE(
registeredPrefs.count(browsing_data::prefs::kDeleteBrowsingHistory));
EXPECT_TRUE(registeredPrefs.count(browsing_data::prefs::kCloseTabs));
EXPECT_TRUE(registeredPrefs.count(browsing_data::prefs::kDeleteCache));
EXPECT_TRUE(registeredPrefs.count(browsing_data::prefs::kDeleteFormData));
// The counter for passwords should NOT be created.
EXPECT_FALSE(registeredPrefs.count(browsing_data::prefs::kDeletePasswords));You can use `testing::UnorderedElementsAre` to validate the content of the set:
```suggestion
EXPECT_THAT(registered_prefs, testing::UnorderedElementsAre(
browsing_data::prefs::kDeleteBrowsingHistory,
browsing_data::prefs::kCloseTabs,
browsing_data::prefs::kDeleteCache,
browsing_data::prefs::kDeleteFormData));
```
Same for the test below
// Expect counters for history, tabs, cache and autofill.```suggestion
// Expect counters for history, tabs, cache, autofill and passwords.
```
return _registeredPrefNames;Why not just construct the set here by iterating over `_prefsCallback`? This would remove the need to call `clearRegisteredPrefs` to clear previous states. Also, do we need to clear `_prefsCallback`? A new `FakeBrowsingDataCounterWrapperProducer` should be created for every test case
Related details
- Angela Novakovic
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. |
Angela Novakovic (Gerrit)
Angela Novakovic added 4 comments![]( "Open in Gerrit")
`registered_prefs` as this is C++
Done
EXPECT_TRUE(
registeredPrefs.count(browsing_data::prefs::kDeleteBrowsingHistory));
EXPECT_TRUE(registeredPrefs.count(browsing_data::prefs::kCloseTabs));
EXPECT_TRUE(registeredPrefs.count(browsing_data::prefs::kDeleteCache));
EXPECT_TRUE(registeredPrefs.count(browsing_data::prefs::kDeleteFormData));
// The counter for passwords should NOT be created.
EXPECT_FALSE(registeredPrefs.count(browsing_data::prefs::kDeletePasswords));You can use `testing::UnorderedElementsAre` to validate the content of the set:
```suggestion
EXPECT_THAT(registered_prefs, testing::UnorderedElementsAre(
browsing_data::prefs::kDeleteBrowsingHistory,
browsing_data::prefs::kCloseTabs,
browsing_data::prefs::kDeleteCache,
browsing_data::prefs::kDeleteFormData));
```Same for the test below
Done
// Expect counters for history, tabs, cache and autofill.```suggestion
// Expect counters for history, tabs, cache, autofill and passwords.
```
Done
Why not just construct the set here by iterating over `_prefsCallback`? This would remove the need to call `clearRegisteredPrefs` to clear previous states. Also, do we need to clear `_prefsCallback`? A new `FakeBrowsingDataCounterWrapperProducer` should be created for every test case
- Noémie St-Onge
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Noémie St-Onge (Gerrit)
Noémie St-Onge added 6 comments![]( "Open in Gerrit")
#import "ios/chrome/browser/browsing_data/model/browsing_data_remove_mask.h"
#import "ios/chrome/browser/browsing_data/model/browsing_data_remover.h"Are these imports needed?
feature_list.InitAndDisableFeature(kPasswordRemovalFromDeleteBrowsingData);We can skip a line after enabling or disabling the feature. It makes the code easier to parse. Same for the other tests below
EXPECT_FALSE(registered_prefs.count(browsing_data::prefs::kDeletePasswords));No need to check this as the above check with `UnorderedElementsAre` would have failed if `kDeletePasswords` was part of the registered prefs. We can take the `The counter for passwords should NOT be created.` comment and append it to the `Expect counters for history, tabs, cache and autofill.` comment
// `kPasswordRemovalFromDeleteBrowsingData` is disabled.The start of the comment seems to have been removed
// Expect counters for history, tabs, cache and autofill.```suggestion
// Expect counters for history, tabs, cache, autofill and passwords.
```
std::set<std::string> _registeredPrefNames;I don't think we need to store this variable
Related details
- Angela Novakovic
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. |
Angela Novakovic (Gerrit)
Angela Novakovic added 6 comments![]( "Open in Gerrit")
#import "ios/chrome/browser/browsing_data/model/browsing_data_remove_mask.h"
#import "ios/chrome/browser/browsing_data/model/browsing_data_remover.h"Angela NovakovicAre these imports needed?
Done
feature_list.InitAndDisableFeature(kPasswordRemovalFromDeleteBrowsingData);We can skip a line after enabling or disabling the feature. It makes the code easier to parse. Same for the other tests below
Done
EXPECT_FALSE(registered_prefs.count(browsing_data::prefs::kDeletePasswords));No need to check this as the above check with `UnorderedElementsAre` would have failed if `kDeletePasswords` was part of the registered prefs. We can take the `The counter for passwords should NOT be created.` comment and append it to the `Expect counters for history, tabs, cache and autofill.` comment
Done
// `kPasswordRemovalFromDeleteBrowsingData` is disabled.The start of the comment seems to have been removed
Done
// Expect counters for history, tabs, cache and autofill.```suggestion
// Expect counters for history, tabs, cache, autofill and passwords.
```
Done
I don't think we need to store this variable
- Noémie St-Onge
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |