New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 856696 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Nov 22
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Task



Sign in to add a comment

Add support for PRE_ browser tests that span browser restarts

Project Member Reported by mastiz@chromium.org, Jun 26 2018

Issue description

Currently, sync integration tests are not compatible with the test infrastructure's ability to mimic browser restarts as described in "Tests Spanning Restarts" in https://www.chromium.org/developers/testing/browser-tests

The reason is that sync integration tests create a random unique temporary directory: https://cs.chromium.org/chromium/src/chrome/browser/sync/test/integration/sync_test.cc?l=349&rcl=eb937ba1802a2741f49c7a6b25424e9727ab2159

The result is that we can hardly test sync restarting, and even the tests that do that (e.g. TwoClientPollingSyncTest) do it in a way that diverges considerably from the real flow (where KeyedServices, I/O, etc. are stopped).

Fixing this could be as easy as making the profile directory name deterministic.
 
Labels: sync-fixit-2018q3
sync-triage ping: any updates?
sync-triage-ping, any updates?
Labels: sync-fixit-2018q4
Owner: mastiz@chromium.org
Status: Assigned (was: Available)
Status: Started (was: Assigned)
Project Member

Comment 7 by bugdroid1@chromium.org, Nov 21

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/938efa5c2de64ce6cf3e88050920175a40321de2

commit 938efa5c2de64ce6cf3e88050920175a40321de2
Author: Mikel Astiz <mastiz@chromium.org>
Date: Wed Nov 21 19:28:29 2018

Add support for sync integration tests that span restarts

Historically, sync integration tests were incompatible with testing an
actual browser restarts, leading to (best case) weird workarounds or
(more often) lack of test coverage for very basic functionality (e.g.
loading of persisted data).

Browser tests do support a mechanism for this, which involves prefixing
test names with PRE_, designed to reuse the very same profile path in
multiple tests and hence carry over state.

The only reason why this didn't work for sync integration tests is that
we created random directories on every test run: instead, this patch
makes the profile path deterministic.

In this patch, a couple of tests are migrated to the new scheme, as
a proof-of-concept.

Bug:  856696 
Change-Id: I6bf33bcb78d834984ff5230f7119cb79eb8bab55
Reviewed-on: https://chromium-review.googlesource.com/c/1341996
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Mihai Sardarescu <msarda@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Reviewed-by: Colin Blundell <blundell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#610152}
[modify] https://crrev.com/938efa5c2de64ce6cf3e88050920175a40321de2/chrome/browser/extensions/api/identity/identity_apitest.cc
[modify] https://crrev.com/938efa5c2de64ce6cf3e88050920175a40321de2/chrome/browser/sync/test/integration/enable_disable_test.cc
[modify] https://crrev.com/938efa5c2de64ce6cf3e88050920175a40321de2/chrome/browser/sync/test/integration/single_client_polling_sync_test.cc
[modify] https://crrev.com/938efa5c2de64ce6cf3e88050920175a40321de2/chrome/browser/sync/test/integration/single_client_user_consents_sync_test.cc
[modify] https://crrev.com/938efa5c2de64ce6cf3e88050920175a40321de2/chrome/browser/sync/test/integration/single_client_user_events_sync_test.cc
[modify] https://crrev.com/938efa5c2de64ce6cf3e88050920175a40321de2/chrome/browser/sync/test/integration/sync_test.cc
[modify] https://crrev.com/938efa5c2de64ce6cf3e88050920175a40321de2/chrome/browser/sync/test/integration/sync_test.h
[modify] https://crrev.com/938efa5c2de64ce6cf3e88050920175a40321de2/services/identity/public/cpp/identity_test_utils.cc

Status: Fixed (was: Started)
Project Member

Comment 9 by bugdroid1@chromium.org, Nov 22

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2a4004c78dc1f90564cc77653c1641b6813a17f8

commit 2a4004c78dc1f90564cc77653c1641b6813a17f8
Author: Mikel Astiz <mastiz@chromium.org>
Date: Thu Nov 22 11:58:29 2018

Add more sync tests than span browser restarts

They verify the very basic requirement of progress markers being
peristed and loaded upon restart, which prevents clients to fetch all
data from scratch from the server.

This also surfaces a bug with USS bookmarks that should be addressed in
a follow-up patch.

Bug:  856696 
Change-Id: I924249579f2d5790f25be6809b03f50d5c421ffc
Reviewed-on: https://chromium-review.googlesource.com/c/1347366
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Mohamed Amir Yosef <mamir@chromium.org>
Cr-Commit-Position: refs/heads/master@{#610385}
[modify] https://crrev.com/2a4004c78dc1f90564cc77653c1641b6813a17f8/chrome/browser/sync/test/integration/single_client_bookmarks_sync_test.cc
[modify] https://crrev.com/2a4004c78dc1f90564cc77653c1641b6813a17f8/chrome/browser/sync/test/integration/single_client_polling_sync_test.cc
[modify] https://crrev.com/2a4004c78dc1f90564cc77653c1641b6813a17f8/chrome/browser/sync/test/integration/single_client_preferences_sync_test.cc

Project Member

Comment 10 by bugdroid1@chromium.org, Jan 16 (6 days ago)

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5253c803e3e60330a0f444fe61b97ac8fc82ff46

commit 5253c803e3e60330a0f444fe61b97ac8fc82ff46
Author: Eduard Satdarov <sath@yandex-team.ru>
Date: Wed Jan 16 18:32:59 2019

Add support PRE_ tests in FakeServer for sync_integration_tests.

Use fixed subfolder in DIR_USER_DATA for storing perstent state of
LoopbackServer.

Bug:  856696 
Change-Id: I8a934e36ca41e38d02140e329de5494617be1d65
Reviewed-on: https://chromium-review.googlesource.com/c/1409191
Reviewed-by: Martin Šrámek <msramek@chromium.org>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Commit-Queue: Alexander Yashkin <a-v-y@yandex-team.ru>
Cr-Commit-Position: refs/heads/master@{#623306}
[modify] https://crrev.com/5253c803e3e60330a0f444fe61b97ac8fc82ff46/chrome/browser/browsing_data/counters/sync_aware_counter_browsertest.cc
[modify] https://crrev.com/5253c803e3e60330a0f444fe61b97ac8fc82ff46/chrome/browser/sync/test/integration/single_client_secondary_account_sync_test.cc
[modify] https://crrev.com/5253c803e3e60330a0f444fe61b97ac8fc82ff46/chrome/browser/sync/test/integration/single_client_wallet_sync_test.cc
[modify] https://crrev.com/5253c803e3e60330a0f444fe61b97ac8fc82ff46/chrome/browser/sync/test/integration/sync_test.cc
[modify] https://crrev.com/5253c803e3e60330a0f444fe61b97ac8fc82ff46/components/sync/test/fake_server/fake_server.cc
[modify] https://crrev.com/5253c803e3e60330a0f444fe61b97ac8fc82ff46/components/sync/test/fake_server/fake_server.h

Sign in to add a comment