-
Notifications
You must be signed in to change notification settings - Fork 37
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update tests to cover unexpected lower-the-rollout scenario #1224
Conversation
Tests/BrowserServicesKitTests/PrivacyConfig/AppPrivacyConfigurationTests.swift
Outdated
Show resolved
Hide resolved
Tests/BrowserServicesKitTests/PrivacyConfig/AppPrivacyConfigurationTests.swift
Outdated
Show resolved
Hide resolved
Tests/BrowserServicesKitTests/PrivacyConfig/AppPrivacyConfigurationTests.swift
Show resolved
Hide resolved
XCTAssertEqual(config.stateFor(AutofillSubfeature.credentialsSaving), .enabled) | ||
} | ||
|
||
func testWhenCheckingSubfeatureStateAndRolloutStepChanges_SubfeatureMatchesOldDisabledResult() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got a question. For this rollout, will we always replace the value in the configuration instead of adding other rollout steps?
I can see that we load the embeddedData always with one item.
I was looking at the isRolloutEnabled
code and If we add an other item to the phase e.g.
"rollout": {
"steps": [
{
"percent": 50.0
},
{
"percent": 5.0
}
]
}
there’s a chance that the feature is re-enabled?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Subsequent steps should always be higher than the previous one, and each new step gives a chance for a device to be allocated in the rollout.
So if we change [50] to [5] then we can continue rollout by doing [5, 10] then [5, 10, 20] etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha.
That means if we were to implement this hypotetic plan the percentages would map to those steps.
Day | Day of the Week | App Store Rollout | Remote Rollout (percentage) | Remote Rollout (steps) |
---|---|---|---|---|
Day 1 | Monday | 1% | 50% | [50] |
Day 2 | Tuesday | 2% | 50% | [50] |
Day 3 | Wednesday | 5% -> 100% | 50% -> 5% | [5] |
Day 4 | Thursday | 100% | 10% | [5, 10] |
Day 5 | Friday | 100% | 25% | [5, 10, 25] |
Day 6 | Saturday | 100% | 25% | [5, 10, 25] |
Day 7 | Sunday | 100% | 25% | [5, 10, 25] |
Day 8 | Monday | 100% | 50% | [5, 10, 25, 50] |
Day 9 | Tuesday | 100% | 100% | [5, 10, 25, 50, 100] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that should work out. Just make sure to update the config before bumping the release to 100% :)
Co-authored-by: Alessandro Boron <[email protected]>
Co-authored-by: Alessandro Boron <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks for veryfing this!
Please review the release process for BrowserServicesKit here.
Required:
Task/Issue URL: https://app.asana.com/0/0/1209336234197627/f
iOS PR: n/a
macOS PR: n/a
What kind of version bump will this require?: Patch
Description:
See https://app.asana.com/0/0/1209336234197627/f for discussion.
Steps to test this PR:
Validate is scenario meets criteria outlined in the task.
Internal references:
Software Engineering Expectations
Technical Design Template