Fix first run UI test when we await the sync startup [chromium/src : main]
Anthi Orfanou (Gerrit)
Anthi Orfanou voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Ryan Sultanem
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. |
Anthi Orfanou (Gerrit)
Anthi Orfanou added 1 comment![]( "Open in Gerrit")
test_sync_service->SetMaxTransportState(Anthi Orfanoui think this should pass but skips the waiter. can we add a successful test case WITH the waiter (trigger at timeout)?
Done
Related details
- Ryan Sultanem
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ryan Sultanem (Gerrit)
Ryan Sultanem voted and added 3 comments![]( "Open in Gerrit")
Votes added by Ryan Sultanem
| Code-Review | +1 |
3 comments
LGTM % questions. I left some minor questions.
Thanks!
.with_sync_engine_ready = false},Is it safe not to force enable the feature in the test suite?
Considering that there are bots post commit that run the tests without considering the fieldtrial settings.
"UnoPhase2TrackSyncStartupStateForDesktop": [Question: Is this intended to be launched separately? Or can it be included as part of the fast follows as it is only a technical change?
Related details
- Anthi Orfanou
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. |
Anthi Orfanou (Gerrit)
Anthi Orfanou added 2 comments![]( "Open in Gerrit")
.with_sync_engine_ready = false},Is it safe not to force enable the feature in the test suite?
Considering that there are bots post commit that run the tests without considering the fieldtrial settings.
I think we are fine because the bots without field trial will test the old (pre-uno flow) anyway, and the bots with field trial will run the uno flow + this new feature.
This test suite is already adjusted for workign with both enabled/disabled uno flags (ReplaceSyncPromosWithSigninPromos).
"UnoPhase2TrackSyncStartupStateForDesktop": [Question: Is this intended to be launched separately? Or can it be included as part of the fast follows as it is only a technical change?
We plan to just enable by default the feature, which I need to do in an upcoming CL (it will affect only uno users as the flag is only consumed in the history sync optin helper - used in uno phase 2 only).
In the meantime I wanted to check that we have some test coverage through the field trial config.
Related details
- Ryan Sultanem
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. |
Ryan Sultanem (Gerrit)
Ryan Sultanem added 2 comments![]( "Open in Gerrit")
.with_sync_engine_ready = false},Anthi OrfanouIs it safe not to force enable the feature in the test suite?
Considering that there are bots post commit that run the tests without considering the fieldtrial settings.
I think we are fine because the bots without field trial will test the old (pre-uno flow) anyway, and the bots with field trial will run the uno flow + this new feature.
This test suite is already adjusted for workign with both enabled/disabled uno flags (ReplaceSyncPromosWithSigninPromos).
Acknowledged
"UnoPhase2TrackSyncStartupStateForDesktop": [Anthi OrfanouQuestion: Is this intended to be launched separately? Or can it be included as part of the fast follows as it is only a technical change?
We plan to just enable by default the feature, which I need to do in an upcoming CL (it will affect only uno users as the flag is only consumed in the history sync optin helper - used in uno phase 2 only).
In the meantime I wanted to check that we have some test coverage through the field trial config.
Oh so the flag is only meant as a kill-switch?
If so, could we just enable it by default instead of enabling it via fieldtrial?
Related details
- Anthi Orfanou
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. |
Anthi Orfanou (Gerrit)
Anthi Orfanou added 1 comment![]( "Open in Gerrit")
"UnoPhase2TrackSyncStartupStateForDesktop": [Anthi OrfanouQuestion: Is this intended to be launched separately? Or can it be included as part of the fast follows as it is only a technical change?
Ryan SultanemWe plan to just enable by default the feature, which I need to do in an upcoming CL (it will affect only uno users as the flag is only consumed in the history sync optin helper - used in uno phase 2 only).
In the meantime I wanted to check that we have some test coverage through the field trial config.
Oh so the flag is only meant as a kill-switch?
If so, could we just enable it by default instead of enabling it via fieldtrial?
yes, that's the plan (kill switch). Ok, I will re-work this CL to include the enabling part.
Related details
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. |