[sync] Update Sync integration tests to auto-open first tab [chromium/src : main]
Mikel Astiz (Gerrit)
Mikel Astiz voted and added 1 comment![]( "Open in Gerrit")
Votes added by Mikel Astiz
| Commit-Queue | +1 |
1 comment
Related details
- Ankush Singh
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ankush Singh (Gerrit)
Ankush Singh added 8 comments![]( "Open in Gerrit")
This behavior ressembles more closely what regular browser tests too,do?
to their expectations, as they could the number of tabs. With antypo?
EXPECT_THAT(GetFakeServer()->GetSyncEntitiesByDataType(syncer::BOOKMARKS),
Contains(Not(HasUniquePosition())).Times(2));Why does this expectation not hold true?
// TODO(crbug.com/356148174): Change the production logic to not uninstallThis links to an already-closed bug. Maybe create a new one?
Also I'm unclear as to what's actually happening: When a new extension theme is installed we show an infobar to undo the applied theme. Are we closing that?
CloseAllInfoBars(GetProfile(0));Can we instead move this to UseCustomTheme() instead?
browsers_.push_back(Browser::Create(Browser::CreateParams(profile, true)));
DCHECK_EQ(static_cast<size_t>(index), browsers_.size() - 1);
Browser* browser = browsers_.back();
chrome::AddSelectedTabWithURL(browser, GetInitialURL(),
ui::PAGE_TRANSITION_AUTO_TOPLEVEL);
// Show the browser window. Otherwise, the rendering pipeline might not
// initialize or produce frames (e.g., on Wayland headless bots), which
// can cause tests relying on hit test data or visual state to time out.
browser->window()->Show();optional: this code is almost the same as in AddBrowser() except the latter also adds the profile? Maybe we can dedup some of the code?
tab_count = 2;nit: maybe add a comment here to explain why it's 2 and not 1?
Related details
- Mikel Astiz
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. |
Mikel Astiz (Gerrit)
Mikel Astiz added 8 comments![]( "Open in Gerrit")
This behavior ressembles more closely what regular browser tests too,Mikel Astizdo?
Done
to their expectations, as they could the number of tabs. With anMikel Astiztypo?
Done
EXPECT_THAT(GetFakeServer()->GetSyncEntitiesByDataType(syncer::BOOKMARKS),
Contains(Not(HasUniquePosition())).Times(2));Why does this expectation not hold true?
As mentioned in the CL description, these tests expectations are fragile in the sense that they make assumptions about how other datatypes behave, including sessions. If sessions trigger a commit, the unique position is uploaded slightly earlier.
// TODO(crbug.com/356148174): Change the production logic to not uninstallThis links to an already-closed bug. Maybe create a new one?
Also I'm unclear as to what's actually happening: When a new extension theme is installed we show an infobar to undo the applied theme. Are we closing that?
This links to an already-closed bug. Maybe create a new one?
Done.
Also I'm unclear as to what's actually happening: When a new extension theme is installed we show an infobar to undo the applied theme. Are we closing that?
It's also not very clear to me. But from what I can see, there is a pre-existing issue in tests, which is surfaced now because the notification UI influences the timing for `ThemeService::RemoveUnusedThemes()`, and the constraint that themes cannot be reinstalled automatically in tests (update URL is empty).
CloseAllInfoBars(GetProfile(0));Can we instead move this to UseCustomTheme() instead?
I believe that would also require moving the waiting logic, which seems undesirable. It would become something like `UseCustomThemeAndWait()`. Is this what you would prefer?
Besides, if we do this for custom themes, we should for consistency also do it for other kinds of themes, e.g. `UseSystemTheme`. WDYT?
browsers_.push_back(Browser::Create(Browser::CreateParams(profile, true)));
DCHECK_EQ(static_cast<size_t>(index), browsers_.size() - 1);
Browser* browser = browsers_.back();
chrome::AddSelectedTabWithURL(browser, GetInitialURL(),
ui::PAGE_TRANSITION_AUTO_TOPLEVEL);
// Show the browser window. Otherwise, the rendering pipeline might not
// initialize or produce frames (e.g., on Wayland headless bots), which
// can cause tests relying on hit test data or visual state to time out.
browser->window()->Show();optional: this code is almost the same as in AddBrowser() except the latter also adds the profile? Maybe we can dedup some of the code?
This code will change in the next patch, so we can continue the discussion there.
tab_count = 2;nit: maybe add a comment here to explain why it's 2 and not 1?
I don't actually know how these numbers can be justified, except the fact that they need to increase by one with this patch. I will ask owners for their input and guidance.
Related details
- Ankush Singh
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. |
Ankush Singh (Gerrit)
Ankush Singh voted and added 3 comments![]( "Open in Gerrit")
Votes added by Ankush Singh
| Code-Review | +1 |
3 comments
Thanks! LGTM % open comments.
EXPECT_THAT(GetFakeServer()->GetSyncEntitiesByDataType(syncer::BOOKMARKS),
Contains(Not(HasUniquePosition())).Times(2));Mikel AstizWhy does this expectation not hold true?
As mentioned in the CL description, these tests expectations are fragile in the sense that they make assumptions about how other datatypes behave, including sessions. If sessions trigger a commit, the unique position is uploaded slightly earlier.
Acknowledged
CloseAllInfoBars(GetProfile(0));Mikel AstizCan we instead move this to UseCustomTheme() instead?
I believe that would also require moving the waiting logic, which seems undesirable. It would become something like `UseCustomThemeAndWait()`. Is this what you would prefer?
Besides, if we do this for custom themes, we should for consistency also do it for other kinds of themes, e.g. `UseSystemTheme`. WDYT?
I think a `UseCustomThemeAndWaitForApply()` would be good. But we should wait for the theme to be applied rather that this wait with ServerThemeMatchChecker(). That is, `CustomThemeChecker(GetProfile(0)).Wait()`.
Besides, if we do this for custom themes, we should for consistency also do it for other kinds of themes, e.g. UseSystemTheme. WDYT?
Hmm, I don't think that's really necessary. IIRC all the other themes get applied instantaneously except for extension themes.
Related details
- Mikel Astiz
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. |
Mikel Astiz (Gerrit)
Mikel Astiz added 1 comment![]( "Open in Gerrit")
tab_count = 2;Mikel Astiznit: maybe add a comment here to explain why it's 2 and not 1?
I don't actually know how these numbers can be justified, except the fact that they need to increase by one with this patch. I will ask owners for their input and guidance.
vinny...@google.com can you help me understand (and perhaps document in code) where these numbers are coming from?
Related details
- Ankush Singh
- Varun Khaneja
- Vinny Persky
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. |