Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Conversation

@chinmaygarde
Copy link
Member

The tests we write must be resilient to the order in which they are run in the
harness. That is, they must not rely on global state set by other tests that
have already run in the process. Also, these tests must themselves be
repeatable. That is, they must correctly clean up after themselves and be able
to run successfully again in the same process.

This patch adds some safeguards against (but does NOT guarantee) the addition of
tests that violate the dictum.

Additionally, test failures must be easily reproducible for folks investigating
the test failure. Also, tests that assert correctness of unrelated code must not
stop progress on the authors patch.

This changes does not hinder reproducibility of test failures because the random
seed is printed in the logs before running each test. Developers attempting to
reproduce the failure locally can do the same via the following invocation
--gtest_shuffle --gtest_repeat=<the count> --gtest_random_seed=<seed from failing run>.

This change does introduce potential burden on patch authors that may see
failures in unrelated code as a newly failing shuffle seed is used on their
runs. To ameliorate this, we will formulate guidance for them to aggressively
mark such tests as disabled and file bugs to enable the same.

The test seed is intentionally kept low because it’s purpose is to test that
individual tests are repeatable. It must not be used as a replacement for
fuzzing.

Copy link
Contributor

@dnfield dnfield left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

The tests we write must be resilient to the order in which they are run in the
harness. That is, they must not rely on global state set by other tests that
have already run in the process. Also, these tests must themselves be
repeatable. That is, they must correctly clean up after themselves and be able
to run successfully again in the same process.

This patch adds some safeguards against (but does NOT guarantee) the addition of
tests that violate the dictum.

Additionally, test failures must be easily reproducible for folks investigating
the test failure. Also, tests that assert correctness of unrelated code must not
stop progress on the authors patch.

This changes does not hinder reproducibility of test failures because the random
seed is printed in the logs before running each test. Developers attempting to
reproduce the failure locally can do the same via the following invocation
`--gtest_shuffle --gtest_repeat=<the count> --gtest_random_seed=<seed from failing run>`.

This change does introduce potential burden on patch authors that may see
failures in unrelated code as a newly failing shuffle seed is used on their
runs. To ameliorate this, we will formulate guidance for them to aggressively
mark such tests as disabled and file bugs to enable the same.

The test seed is intentionally kept low because it’s purpose is to test that
individual tests are repeatable. It must not be used as a replacement for
fuzzing.
@dnfield
Copy link
Contributor

dnfield commented Sep 13, 2019

(The "fail" thin gwas to hold the tree closed while mklim and aam were sorting out Dart breakages - it's safe to ignore at this point, has been reverted)

@chinmaygarde chinmaygarde merged commit b4d8158 into flutter:master Sep 17, 2019
@chinmaygarde chinmaygarde deleted the random_shuffle branch September 17, 2019 21:50
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 18, 2019
engine-flutter-autoroll added a commit to flutter/flutter that referenced this pull request Sep 18, 2019
[email protected]:flutter/engine.git/compare/d1692d4cc703...6a96417

git log d1692d4..6a96417 --no-merges --oneline
2019-09-17 [email protected] Roll fuchsia/sdk/core/linux-amd64 from RRgw-... to F-g18... (flutter/engine#12326)
2019-09-17 [email protected] Account for root surface transformation on the surfaces managed by the external view embedder. (flutter/engine#11384)
2019-09-17 [email protected] Introduce FlutterFragmentActivity (flutter/engine#12305)
2019-09-17 [email protected] Shuffle test order and repeat test runs once. (flutter/engine#12275)


If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC [email protected] on the revert to ensure that a human
is aware of the problem.

To report a problem with the AutoRoller itself, please file a bug:
https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md
Inconnu08 pushed a commit to Inconnu08/flutter that referenced this pull request Sep 30, 2019
[email protected]:flutter/engine.git/compare/d1692d4cc703...6a96417

git log d1692d4..6a96417 --no-merges --oneline
2019-09-17 [email protected] Roll fuchsia/sdk/core/linux-amd64 from RRgw-... to F-g18... (flutter/engine#12326)
2019-09-17 [email protected] Account for root surface transformation on the surfaces managed by the external view embedder. (flutter/engine#11384)
2019-09-17 [email protected] Introduce FlutterFragmentActivity (flutter/engine#12305)
2019-09-17 [email protected] Shuffle test order and repeat test runs once. (flutter/engine#12275)


If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC [email protected] on the revert to ensure that a human
is aware of the problem.

To report a problem with the AutoRoller itself, please file a bug:
https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants