Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
20545002:
[Android] Add 'official_build' option for running an instrumentation suite. (Closed)
|
Created:
7 years, 5 months ago by Siva Chandra Modified:
7 years, 4 months ago CC:
chromium-reviews, craigdh+watch_chromium.org, bulach+watch_chromium.org, yfriedman+watch_chromium.org, ilevy-cc_chromium.org, klundberg+watch_chromium.org, frankf+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Description[Android] Add 'official_build' option for running an instrumentation suite.
BUG=249997
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=219401
Patch Set 1 #Patch Set 2 : Add doc #
Total comments: 2
Patch Set 3 : Consise cmd line arg #Messages
Total messages: 13 (0 generated)
This is required for the downstream official builders.
On 2013/07/25 22:20:16, Siva Chandra wrote: > This is required for the downstream official builders. ping? This has now become urgent: https://code.google.com/p/chromium/issues/detail?id=168826
Do these options exist upstream? This (and all other) args should only be present in the repository which need it. So if it's a downstream only arg then you need to override this function downstream. https://chromiumcodereview.appspot.com/20545002/diff/3001/build/android/build... File build/android/buildbot/bb_device_steps.py (right): https://chromiumcodereview.appspot.com/20545002/diff/3001/build/android/build... build/android/buildbot/bb_device_steps.py:195: args.append('--official-build=True') Can we change it to action='store_true' in this CL?
https://chromiumcodereview.appspot.com/20545002/diff/3001/build/android/build... File build/android/buildbot/bb_device_steps.py (right): https://chromiumcodereview.appspot.com/20545002/diff/3001/build/android/build... build/android/buildbot/bb_device_steps.py:195: args.append('--official-build=True') On 2013/08/20 02:45:03, Isaac wrote: > Can we change it to action='store_true' in this CL? It is already setup with that action. Hence made the arg concise in this CL.
On 2013/08/20 02:45:03, Isaac wrote: > Do these options exist upstream? This (and all other) args should only be > present in the repository which need it. So if it's a downstream only arg then > you need to override this function downstream. Overriding is a good idea, but this function actually invokes the test script via RunCmd. If we override it downstream, we will essentially have re-implement most it down stream.
On 2013/08/20 18:54:16, Siva Chandra wrote: > On 2013/08/20 02:45:03, Isaac wrote: > > Do these options exist upstream? This (and all other) args should only be > > present in the repository which need it. So if it's a downstream only arg > then > > you need to override this function downstream. > > Overriding is a good idea, but this function actually invokes the test script > via RunCmd. If we override it downstream, we will essentially have re-implement > most it down stream. Why is it taking a month to land this? This is a regression in downstream infrastructure.
+navabi, xusydoc for review. If Isaac is able to complete the review soon, we can rely on that, but this way we can get some more eyes on this.
On 2013/08/23 18:08:25, frankf wrote: > On 2013/08/20 18:54:16, Siva Chandra wrote: > > On 2013/08/20 02:45:03, Isaac wrote: > > > Do these options exist upstream? This (and all other) args should only be > > > present in the repository which need it. So if it's a downstream only arg > > then > > > you need to override this function downstream. > > > > Overriding is a good idea, but this function actually invokes the test script > > via RunCmd. If we override it downstream, we will essentially have > re-implement > > most it down stream. > > Why is it taking a month to land this? This is a regression in downstream > infrastructure. The bug link does not indicate a regression, so I was not aware of urgency. Implementing an option outside the repo it is used goes against the design of these scripts. We can land this if it's urgently needed but it's not working as intended.
On 2013/08/23 19:19:52, Isaac wrote: > Implementing an option outside the repo it is used goes against the design of > these scripts. We can land this if it's urgently needed but it's not working as > intended. Such a usage is present all over the place. Infact, what you suggested yesterday (https://codereview.chromium.org/22824037/) is very similar. Note that the option is already exposed by the upstream test scripts even though it is not used upstream.
On 2013/08/23 19:25:42, Siva Chandra wrote: > On 2013/08/23 19:19:52, Isaac wrote: > > Implementing an option outside the repo it is used goes against the design of > > these scripts. We can land this if it's urgently needed but it's not working > as intended. I don't quite understand Isaac's concern. The --official-build argument is already in build/android/test_runner.py, which is upstream. So this CL is not introducing the option upstream, only using it. If it should be moved downstream from test_runner.py, then I don't think that should be a requirement for this CL. I am not a committer, but this LGTM.
right, I think this is OK then. lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sivachandra@chromium.org/20545002/8001
Message was sent while issue was closed.
Change committed as 219401 |
