Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11437018:
Change test running logic to run tests without any size annotation. (Closed)
|
Created:
8 years ago by shashi Modified:
8 years ago CC:
chromium-reviews, ilevy+watch_chromium.org, bulach+watch_chromium.org, yfriedman+watch_chromium.org, peter+watch_chromium.org, klundberg+watch_chromium.org, frankf+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionChange test running logic to run tests without any size annotation.
Currently some tests without any test size annotation but with any other
annotation (like PhoneOnly) are not run. These tests are not disabled and
should be run, all disabled tests should now have a DisabledTest annotation.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=171434
Patch Set 1 : #
Total comments: 5
Patch Set 2 : apply feedback #
Total comments: 8
Patch Set 3 : #Patch Set 4 : #Messages
Total messages: 10 (0 generated)
+Frank who's taken ownership of these scripts https://chromiumcodereview.appspot.com/11437018/diff/3001/build/android/pylib... File build/android/pylib/run_java_tests.py (right): https://chromiumcodereview.appspot.com/11437018/diff/3001/build/android/pylib... build/android/pylib/run_java_tests.py:536: default_size_annotation = 'FlakyTest' I think I prefer unspecified to not be Flaky. In general, we should not have flaky tests and shouldn't plan for them. https://chromiumcodereview.appspot.com/11437018/diff/3001/build/android/pylib... build/android/pylib/run_java_tests.py:544: if frozenset(test_apk.GetTestAnnotations(m)). this is now too complicated for a list comprehension. Please extract a helper function
https://chromiumcodereview.appspot.com/11437018/diff/3001/build/android/pylib... File build/android/pylib/run_java_tests.py (right): https://chromiumcodereview.appspot.com/11437018/diff/3001/build/android/pylib... build/android/pylib/run_java_tests.py:536: default_size_annotation = 'FlakyTest' Agree with Yaron. If someone adds a test and forgets to annotate it, then it will get lost in the "miscellaneous bucket" we call flaky tests until the next fixit.
https://chromiumcodereview.appspot.com/11437018/diff/3001/build/android/pylib... File build/android/pylib/run_java_tests.py (right): https://chromiumcodereview.appspot.com/11437018/diff/3001/build/android/pylib... build/android/pylib/run_java_tests.py:536: default_size_annotation = 'FlakyTest' I have it as FlakyTest so that the bots do not break if a new test without annotation is added.I have changed it back to SmallTest. https://chromiumcodereview.appspot.com/11437018/diff/3001/build/android/pylib... build/android/pylib/run_java_tests.py:544: if frozenset(test_apk.GetTestAnnotations(m)). On 2012/12/05 21:52:50, Yaron wrote: > this is now too complicated for a list comprehension. Please extract a helper > function Done.
https://chromiumcodereview.appspot.com/11437018/diff/11001/build/android/pyli... File build/android/pylib/run_java_tests.py (right): https://chromiumcodereview.appspot.com/11437018/diff/11001/build/android/pyli... build/android/pylib/run_java_tests.py:535: def testsMissingAnnotation(test_apk): Unfortunately, we don't follow the google3 style guide for method names. -> _GetTestsMissingAnnotation() https://chromiumcodereview.appspot.com/11437018/diff/11001/build/android/pyli... build/android/pylib/run_java_tests.py:550: options.annotation[0] == default_size_annotation): Why do we include un-annotated tests in SmallTest only if it's run by itself? These tests are left out if no annotation/filter is specified (default behaviour). https://chromiumcodereview.appspot.com/11437018/diff/11001/build/android/pyli... build/android/pylib/run_java_tests.py:553: tests_missing_annotations.sort() just have the method sort it for you.
https://chromiumcodereview.appspot.com/11437018/diff/11001/build/android/pyli... File build/android/pylib/run_java_tests.py (right): https://chromiumcodereview.appspot.com/11437018/diff/11001/build/android/pyli... build/android/pylib/run_java_tests.py:535: def testsMissingAnnotation(test_apk): On 2012/12/05 23:54:17, frankf wrote: > Unfortunately, we don't follow the google3 style guide for method names. -> > _GetTestsMissingAnnotation() Done. https://chromiumcodereview.appspot.com/11437018/diff/11001/build/android/pyli... build/android/pylib/run_java_tests.py:550: options.annotation[0] == default_size_annotation): Currently build bots always specify size when running tests. So tests lacking annotation are never run. Ideally there should be no tests without any annotations. But there has been a few. I saw that this logic has been in place to run un-annotated tests as small tests, except that it does not work when a test has an annotation like PhoneOnly but not size annotation. I thought to improve this logic so that if someone accidently disables a test by removing an annotation we still run it. On 2012/12/05 23:54:17, frankf wrote: > Why do we include un-annotated tests in SmallTest only if it's run by itself? > These tests are left out if no annotation/filter is specified (default > behaviour). https://chromiumcodereview.appspot.com/11437018/diff/11001/build/android/pyli... build/android/pylib/run_java_tests.py:553: tests_missing_annotations.sort() On 2012/12/05 23:54:17, frankf wrote: > just have the method sort it for you. Done.
LGTM with nit. https://chromiumcodereview.appspot.com/11437018/diff/11001/build/android/pyli... File build/android/pylib/run_java_tests.py (right): https://chromiumcodereview.appspot.com/11437018/diff/11001/build/android/pyli... build/android/pylib/run_java_tests.py:550: options.annotation[0] == default_size_annotation): What I meant: 1. Unannotated tests are run: ./run_instrumentation.py -A SmallTest 2. Unannotated tests are not run: ./run_instrumentation.py -A SmallTest,LargeTest ./run_instrumentation.py This is awkward since SmallTests is part of both invocations. On 2012/12/06 00:27:26, shashi wrote: > Currently build bots always specify size when running tests. So tests lacking > annotation are never run. > Ideally there should be no tests without any annotations. But there has been a > few. > I saw that this logic has been in place to run un-annotated tests as small > tests, except that it does not work when a test has an annotation like PhoneOnly > but not size annotation. > I thought to improve this logic so that if someone accidently disables a test > by removing an annotation we still run it. > > On 2012/12/05 23:54:17, frankf wrote: > > Why do we include un-annotated tests in SmallTest only if it's run by itself? > > These tests are left out if no annotation/filter is specified (default > > behaviour). >
https://chromiumcodereview.appspot.com/11437018/diff/11001/build/android/pyli... File build/android/pylib/run_java_tests.py (right): https://chromiumcodereview.appspot.com/11437018/diff/11001/build/android/pyli... build/android/pylib/run_java_tests.py:550: options.annotation[0] == default_size_annotation): Makes sense, we need not check if SmallTest is the only annotation. On 2012/12/06 01:01:55, frankf wrote: > What I meant: > > 1. Unannotated tests are run: > ./run_instrumentation.py -A SmallTest > > 2. Unannotated tests are not run: > ./run_instrumentation.py -A SmallTest,LargeTest > ./run_instrumentation.py > > This is awkward since SmallTests is part of both invocations. > > On 2012/12/06 00:27:26, shashi wrote: > > Currently build bots always specify size when running tests. So tests lacking > > annotation are never run. > > Ideally there should be no tests without any annotations. But there has been a > > few. > > I saw that this logic has been in place to run un-annotated tests as small > > tests, except that it does not work when a test has an annotation like > PhoneOnly > > but not size annotation. > > I thought to improve this logic so that if someone accidently disables a test > > by removing an annotation we still run it. > > > > On 2012/12/05 23:54:17, frankf wrote: > > > Why do we include un-annotated tests in SmallTest only if it's run by > itself? > > > These tests are left out if no annotation/filter is specified (default > > > behaviour). > > >
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/shashishekhar@chromium.org/11437018/7004
Message was sent while issue was closed.
Change committed as 171434 |
