[Android] Buildbot changes for EMMA code coverage

[Android] Buildbot changes for EMMA code coverage

One piece of the overall java code coverage change.
See: https://codereview.chromium.org/20210002/

NOTRY=True
BUG=255644
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=219083

[gkanwar1, 2013-08-20 20:59:03]

This is waiting on a few other CLs, but in the interest of time sending this out
for review now. Thanks!

[frankf, 2013-08-20 22:43:37]

https://codereview.chromium.org/23345003/diff/3001/build/android/buildbot/bb_...
File build/android/buildbot/bb_device_steps.py (left):

https://codereview.chromium.org/23345003/diff/3001/build/android/buildbot/bb_...
build/android/buildbot/bb_device_steps.py:59:
constants.CHROMIUM_TEST_SHELL_HOST_DRIVEN_DIR),
Why is this removed?

https://codereview.chromium.org/23345003/diff/3001/build/android/buildbot/bb_...
File build/android/buildbot/bb_device_steps.py (right):

https://codereview.chromium.org/23345003/diff/3001/build/android/buildbot/bb_...
build/android/buildbot/bb_device_steps.py:280: coverage_dir =
os.path.join(CHROME_SRC, 'out', options.target, 'coverage')
We're doing this twice (line 172)

https://codereview.chromium.org/23345003/diff/3001/build/android/buildbot/bb_...
build/android/buildbot/bb_device_steps.py:305: bot_id =
os.environ.get('BUILDBOT_BUILDERNAME')
See options.build_properties.get* instances in this file

https://codereview.chromium.org/23345003/diff/3001/build/android/buildbot/bb_...
build/android/buildbot/bb_device_steps.py:314: os.environ['BOTO_CONFIG'] =
boto_config
Not sure if you want change envir vars here. Is there anyway to pass this on a
command line?

[gkanwar1, 2013-08-20 23:01:06]

Made the suggested fixes and also moved out the upload functionality into its
own function so that native coverage and downstream can use it.

https://codereview.chromium.org/23345003/diff/3001/build/android/buildbot/bb_...
File build/android/buildbot/bb_device_steps.py (left):

https://codereview.chromium.org/23345003/diff/3001/build/android/buildbot/bb_...
build/android/buildbot/bb_device_steps.py:59:
constants.CHROMIUM_TEST_SHELL_HOST_DRIVEN_DIR),
On 2013/08/20 22:43:37, frankf wrote:
> Why is this removed?

Oops, I think this was a mistake in transferring, I'll add it back.

https://codereview.chromium.org/23345003/diff/3001/build/android/buildbot/bb_...
File build/android/buildbot/bb_device_steps.py (right):

https://codereview.chromium.org/23345003/diff/3001/build/android/buildbot/bb_...
build/android/buildbot/bb_device_steps.py:280: coverage_dir =
os.path.join(CHROME_SRC, 'out', options.target, 'coverage')
On 2013/08/20 22:43:37, frankf wrote:
> We're doing this twice (line 172)

Done.

https://codereview.chromium.org/23345003/diff/3001/build/android/buildbot/bb_...
build/android/buildbot/bb_device_steps.py:305: bot_id =
os.environ.get('BUILDBOT_BUILDERNAME')
On 2013/08/20 22:43:37, frankf wrote:
> See options.build_properties.get* instances in this file

Done.

https://codereview.chromium.org/23345003/diff/3001/build/android/buildbot/bb_...
build/android/buildbot/bb_device_steps.py:314: os.environ['BOTO_CONFIG'] =
boto_config
On 2013/08/20 22:43:37, frankf wrote:
> Not sure if you want change envir vars here. Is there anyway to pass this on a
> command line?

I couldn't find another way to do this. This is also what
build/scripts/slave/slave_utils.py uses for it's gsutil functions.

[Isaac (away), 2013-08-20 23:28:10]

Looks fine -- some initial comments.

https://codereview.chromium.org/23345003/diff/8001/build/android/buildbot/bb_...
File build/android/buildbot/bb_device_steps.py (right):

https://codereview.chromium.org/23345003/diff/8001/build/android/buildbot/bb_...
build/android/buildbot/bb_device_steps.py:279:
shutil.rmtree(options.coverage_dir, ignore_errors=True)
not needed for triggered bots.  Also, I'd prefer we avoid silent shell
manipulations.

https://codereview.chromium.org/23345003/diff/8001/build/android/buildbot/bb_...
build/android/buildbot/bb_device_steps.py:285: if options.coverage_bucket:
Does this need to be called from RunInstrumentationTests?

https://codereview.chromium.org/23345003/diff/8001/build/android/buildbot/bb_...
build/android/buildbot/bb_device_steps.py:311: REVISION_FILENAME)
Would be better to use revision from build_properties.  

options.build_properties.get('got_revision', 'testing')

https://codereview.chromium.org/23345003/diff/8001/build/android/buildbot/bb_...
build/android/buildbot/bb_device_steps.py:323: os.environ['AWS_CREDENTIAL_FILE']
= boto_config
These are set automatically on bots; this isn't needed.  But also you cannot
modify os.environ here.

https://codereview.chromium.org/23345003/diff/8001/build/android/buildbot/bb_...
build/android/buildbot/bb_device_steps.py:325:
RunCmd([os.path.join(bb_utils.BB_BUILD_DIR, 'third_party',
let's make the gsutil path a constant in bb_utils.

[gkanwar1, 2013-08-20 23:40:29]

https://codereview.chromium.org/23345003/diff/8001/build/android/buildbot/bb_...
File build/android/buildbot/bb_device_steps.py (right):

https://codereview.chromium.org/23345003/diff/8001/build/android/buildbot/bb_...
build/android/buildbot/bb_device_steps.py:279:
shutil.rmtree(options.coverage_dir, ignore_errors=True)
On 2013/08/20 23:28:10, Isaac wrote:
> not needed for triggered bots.  Also, I'd prefer we avoid silent shell
> manipulations.

Good to know, removed. Not sure what you mean by silent shell manipulations?

https://codereview.chromium.org/23345003/diff/8001/build/android/buildbot/bb_...
build/android/buildbot/bb_device_steps.py:285: if options.coverage_bucket:
On 2013/08/20 23:28:10, Isaac wrote:
> Does this need to be called from RunInstrumentationTests?

Nope. It was there because it was acting sort of as a 'cleanup' for
instrumentation tests, but now that it's broken into its own step, it probably
would best be placed in MainTestWrapper. Moved.

https://codereview.chromium.org/23345003/diff/8001/build/android/buildbot/bb_...
build/android/buildbot/bb_device_steps.py:311: REVISION_FILENAME)
On 2013/08/20 23:28:10, Isaac wrote:
> Would be better to use revision from build_properties.  
> 
> options.build_properties.get('got_revision', 'testing')

Ah, didn't realize that was in build_properties. Done.

https://codereview.chromium.org/23345003/diff/8001/build/android/buildbot/bb_...
build/android/buildbot/bb_device_steps.py:323: os.environ['AWS_CREDENTIAL_FILE']
= boto_config
On 2013/08/20 23:28:10, Isaac wrote:
> These are set automatically on bots; this isn't needed.  But also you cannot
> modify os.environ here.

Good to know, removed.

https://codereview.chromium.org/23345003/diff/8001/build/android/buildbot/bb_...
build/android/buildbot/bb_device_steps.py:325:
RunCmd([os.path.join(bb_utils.BB_BUILD_DIR, 'third_party',
On 2013/08/20 23:28:10, Isaac wrote:
> let's make the gsutil path a constant in bb_utils.

Done.

[frankf, 2013-08-21 00:42:25]

https://codereview.chromium.org/23345003/diff/13001/build/android/buildbot/bb...
File build/android/buildbot/bb_device_steps.py (right):

https://codereview.chromium.org/23345003/diff/13001/build/android/buildbot/bb...
build/android/buildbot/bb_device_steps.py:45: def I(name, apk, apk_package,
test_apk, test_data, coverage=True,
Didn't we decide all test apks should have coverage?

https://codereview.chromium.org/23345003/diff/13001/build/android/buildbot/bb...
build/android/buildbot/bb_device_steps.py:302: def UploadCoverageData(options,
path, coverage_type):
I'd just inline this

https://codereview.chromium.org/23345003/diff/13001/build/android/buildbot/bb...
build/android/buildbot/bb_device_steps.py:306: timehash = hash(time.time())
Perhaps use something like hashlib.sha1(str(random.random())).hexdigest()

https://codereview.chromium.org/23345003/diff/13001/build/android/buildbot/bb...
build/android/buildbot/bb_device_steps.py:313: % (options.coverage_bucket,
coverage_type, bot_id, revision, timehash))
Not sure if having coverage_type in path is useful

[gkanwar1, 2013-08-21 00:55:15]

https://codereview.chromium.org/23345003/diff/13001/build/android/buildbot/bb...
File build/android/buildbot/bb_device_steps.py (right):

https://codereview.chromium.org/23345003/diff/13001/build/android/buildbot/bb...
build/android/buildbot/bb_device_steps.py:45: def I(name, apk, apk_package,
test_apk, test_data, coverage=True,
On 2013/08/21 00:42:26, frankf wrote:
> Didn't we decide all test apks should have coverage?

Yep, this is a holdover from setting just ContentShell to be covered before.
Removed.

https://codereview.chromium.org/23345003/diff/13001/build/android/buildbot/bb...
build/android/buildbot/bb_device_steps.py:302: def UploadCoverageData(options,
path, coverage_type):
On 2013/08/21 00:42:26, frankf wrote:
> I'd just inline this

This is separate because the native coverage is going to add
GenerateNativeCoverageReport that also makes use of this.

https://codereview.chromium.org/23345003/diff/13001/build/android/buildbot/bb...
build/android/buildbot/bb_device_steps.py:306: timehash = hash(time.time())
On 2013/08/21 00:42:26, frankf wrote:
> Perhaps use something like hashlib.sha1(str(random.random())).hexdigest()

Done.

https://codereview.chromium.org/23345003/diff/13001/build/android/buildbot/bb...
build/android/buildbot/bb_device_steps.py:313: % (options.coverage_bucket,
coverage_type, bot_id, revision, timehash))
On 2013/08/21 00:42:26, frankf wrote:
> Not sure if having coverage_type in path is useful

We're going to be having a native and java coverage report which share bot and
revision id, so it's nice to be able to visually distinguish the URLs (otherwise
the only distinguisher is the hash).

[frankf, 2013-08-21 01:05:27]

lgtm

[frankf, 2013-08-21 01:13:45]

Additional nits

https://codereview.chromium.org/23345003/diff/18001/build/android/buildbot/bb...
File build/android/buildbot/bb_device_steps.py (right):

https://codereview.chromium.org/23345003/diff/18001/build/android/buildbot/bb...
build/android/buildbot/bb_device_steps.py:302: def UploadCoverageData(options,
path, coverage_type):
Add docstring

https://codereview.chromium.org/23345003/diff/18001/build/android/buildbot/bb...
build/android/buildbot/bb_device_steps.py:309: (options.coverage_bucket,
coverage_type, bot_id, revision, randhash)])
DRY, create a var for gs path

[gkanwar1, 2013-08-21 01:29:13]

https://codereview.chromium.org/23345003/diff/18001/build/android/buildbot/bb...
File build/android/buildbot/bb_device_steps.py (right):

https://codereview.chromium.org/23345003/diff/18001/build/android/buildbot/bb...
build/android/buildbot/bb_device_steps.py:302: def UploadCoverageData(options,
path, coverage_type):
On 2013/08/21 01:13:45, frankf wrote:
> Add docstring

Done.

https://codereview.chromium.org/23345003/diff/18001/build/android/buildbot/bb...
build/android/buildbot/bb_device_steps.py:309: (options.coverage_bucket,
coverage_type, bot_id, revision, randhash)])
On 2013/08/21 01:13:45, frankf wrote:
> DRY, create a var for gs path

Done.

[gkanwar1, 2013-08-22 16:21:13]

On 2013/08/21 01:29:13, gkanwar1 wrote:
>
https://codereview.chromium.org/23345003/diff/18001/build/android/buildbot/bb...
> File build/android/buildbot/bb_device_steps.py (right):
> 
>
https://codereview.chromium.org/23345003/diff/18001/build/android/buildbot/bb...
> build/android/buildbot/bb_device_steps.py:302: def UploadCoverageData(options,
> path, coverage_type):
> On 2013/08/21 01:13:45, frankf wrote:
> > Add docstring
> 
> Done.
> 
>
https://codereview.chromium.org/23345003/diff/18001/build/android/buildbot/bb...
> build/android/buildbot/bb_device_steps.py:309: (options.coverage_bucket,
> coverage_type, bot_id, revision, randhash)])
> On 2013/08/21 01:13:45, frankf wrote:
> > DRY, create a var for gs path
> 
> Done.

PS 6 passed but was only including APK instrumentation (so only test files).
This was because the coverage.em files under out/<target>/gen were not being
zipped and sent to the triggered tester. To solve this, PS 7 outputs the
metadata and sources.txt files alongside the output in lib.java. ptal, thanks!

[gkanwar1, 2013-08-22 16:22:49]

cc'ing Chris as well, since I talked this over with him.

[frankf, 2013-08-22 17:34:26]

lgtm w/ nit

https://codereview.chromium.org/23345003/diff/25001/build/android/generate_em...
File build/android/generate_emma_html.py (right):

https://codereview.chromium.org/23345003/diff/25001/build/android/generate_em...
build/android/generate_emma_html.py:63: # to a *_sources.txt (these should be
manually cleaned).
Can you add a TODO that this should be removed once there are no old files
around.

[gkanwar1, 2013-08-22 18:09:58]

Fixed nit. Just waiting on fyi triggered tests to go green with the correct
coverage output, then this can land.

https://codereview.chromium.org/23345003/diff/25001/build/android/generate_em...
File build/android/generate_emma_html.py (right):

https://codereview.chromium.org/23345003/diff/25001/build/android/generate_em...
build/android/generate_emma_html.py:63: # to a *_sources.txt (these should be
manually cleaned).
On 2013/08/22 17:34:26, frankf wrote:
> Can you add a TODO that this should be removed once there are no old files
> around.

Done.

[Isaac (away), 2013-08-22 18:10:36]

lgtm from buildbot perspective

https://codereview.chromium.org/23345003/diff/8001/build/android/buildbot/bb_...
File build/android/buildbot/bb_device_steps.py (right):

https://codereview.chromium.org/23345003/diff/8001/build/android/buildbot/bb_...
build/android/buildbot/bb_device_steps.py:279:
shutil.rmtree(options.coverage_dir, ignore_errors=True)
On 2013/08/20 23:40:29, gkanwar1 wrote:
> On 2013/08/20 23:28:10, Isaac wrote:
> > not needed for triggered bots.  Also, I'd prefer we avoid silent shell
> > manipulations.
> 
> Good to know, removed. Not sure what you mean by silent shell manipulations?

I mean, if you're going to delete a folder, print something like
"Removing <>".  Actually it might be better to use RunCmd() because it will both
print the command and skip running it during testing.

[gkanwar1, 2013-08-22 18:12:11]

https://codereview.chromium.org/23345003/diff/8001/build/android/buildbot/bb_...
File build/android/buildbot/bb_device_steps.py (right):

https://codereview.chromium.org/23345003/diff/8001/build/android/buildbot/bb_...
build/android/buildbot/bb_device_steps.py:279:
shutil.rmtree(options.coverage_dir, ignore_errors=True)
On 2013/08/22 18:10:37, Isaac wrote:
> On 2013/08/20 23:40:29, gkanwar1 wrote:
> > On 2013/08/20 23:28:10, Isaac wrote:
> > > not needed for triggered bots.  Also, I'd prefer we avoid silent shell
> > > manipulations.
> > 
> > Good to know, removed. Not sure what you mean by silent shell manipulations?
> 
> I mean, if you're going to delete a folder, print something like
> "Removing <>".  Actually it might be better to use RunCmd() because it will
both
> print the command and skip running it during testing.

Ah, got it. Good to know for the future. For this CL, this is just entirely
removed.

[commit-bot: I haz the power, 2013-08-22 19:15:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gkanwar@chromium.org/23345003/46001

[commit-bot: I haz the power, 2013-08-22 19:56:37]

Change committed as 219083