Made junit coverage-dir take directory, not file.

Made junit coverage-dir take directory, not file.

--coverage-dir option for junit tests should take directory name, not file name.
With the change, we are able to run the command like this:
'out-gn/Debug/bin/run_clank_junit_tests --coverage-dir /tmp/coverage/junit'.
Then clank_junit_test.ec will be created in the junit directory.

BUG=

Committed: https://crrev.com/f9b5e71a566772a1a80d3e670ed584f60bce71df
Cr-Commit-Position: refs/heads/master@{#395936}

[BigBossZhiling, 2016-05-23 22:26:05]

Description was changed from

==========
Made junit coverage-dir take directory, not file.

--coverage-dir option for junit tests should take directory name, not file name.

BUG=
==========

to

==========
Made junit coverage-dir take directory, not file.

--coverage-dir option for junit tests should take directory name, not file name.
With the change, we are able to run the command like this:
'out-gn/Debug/bin/run_clank_junit_tests --coverage-dir /tmp/coverage/junit'.
Then clank_junit_test.ec will be created in the junit directory.

BUG=
==========

[BigBossZhiling, 2016-05-23 22:29:22]

hzl@google.com changed reviewers:
+ jbudorick@chromium.org, mikecase@chromium.org

[jbudorick, 2016-05-24 12:35:18]

https://codereview.chromium.org/2003213002/diff/20001/build/android/pylib/jun...
File build/android/pylib/junit/test_runner.py (right):

https://codereview.chromium.org/2003213002/diff/20001/build/android/pylib/jun...
build/android/pylib/junit/test_runner.py:50:
jvm_args.append('-Demma.coverage.out.file=%s' % os.path.join(
This should check that self._coverage_dir exists and should make it (via
https://docs.python.org/2/library/os.html#os.makedirs) if it doesn't.

[BigBossZhiling, 2016-05-24 17:41:33]

https://codereview.chromium.org/2003213002/diff/20001/build/android/pylib/jun...
File build/android/pylib/junit/test_runner.py (right):

https://codereview.chromium.org/2003213002/diff/20001/build/android/pylib/jun...
build/android/pylib/junit/test_runner.py:50:
jvm_args.append('-Demma.coverage.out.file=%s' % os.path.join(
On 2016/05/24 12:35:18, jbudorick wrote:
> This should check that self._coverage_dir exists and should make it (via
> https://docs.python.org/2/library/os.html#os.makedirs) if it doesn't.

That's a good point, but I just tested on it and it seems like this emma
coverage will automatically create the directory.

[jbudorick, 2016-05-24 19:15:09]

https://codereview.chromium.org/2003213002/diff/20001/build/android/pylib/jun...
File build/android/pylib/junit/test_runner.py (right):

https://codereview.chromium.org/2003213002/diff/20001/build/android/pylib/jun...
build/android/pylib/junit/test_runner.py:50:
jvm_args.append('-Demma.coverage.out.file=%s' % os.path.join(
On 2016/05/24 17:41:33, BigBossZhiling wrote:
> On 2016/05/24 12:35:18, jbudorick wrote:
> > This should check that self._coverage_dir exists and should make it (via
> > https://docs.python.org/2/library/os.html#os.makedirs) if it doesn't.
> 
> That's a good point, but I just tested on it and it seems like this emma
> coverage will automatically create the directory.

I'd still prefer we create it here.

[BigBossZhiling, 2016-05-24 19:28:52]

https://codereview.chromium.org/2003213002/diff/20001/build/android/pylib/jun...
File build/android/pylib/junit/test_runner.py (right):

https://codereview.chromium.org/2003213002/diff/20001/build/android/pylib/jun...
build/android/pylib/junit/test_runner.py:50:
jvm_args.append('-Demma.coverage.out.file=%s' % os.path.join(
On 2016/05/24 19:15:09, jbudorick wrote:
> On 2016/05/24 17:41:33, BigBossZhiling wrote:
> > On 2016/05/24 12:35:18, jbudorick wrote:
> > > This should check that self._coverage_dir exists and should make it (via
> > > https://docs.python.org/2/library/os.html#os.makedirs) if it doesn't.
> > 
> > That's a good point, but I just tested on it and it seems like this emma
> > coverage will automatically create the directory.
> 
> I'd still prefer we create it here.

Done.

[jbudorick, 2016-05-24 19:33:21]

lgtm w nit

https://codereview.chromium.org/2003213002/diff/40001/build/android/pylib/jun...
File build/android/pylib/junit/test_runner.py (right):

https://codereview.chromium.org/2003213002/diff/40001/build/android/pylib/jun...
build/android/pylib/junit/test_runner.py:50: if not
os.path.isdir(self._coverage_dir):
nit: os.path.exists

although I'm not sure how either would work if coverage_dir exists but isn't a
directory

[BigBossZhiling, 2016-05-25 02:00:20]

https://codereview.chromium.org/2003213002/diff/40001/build/android/pylib/jun...
File build/android/pylib/junit/test_runner.py (right):

https://codereview.chromium.org/2003213002/diff/40001/build/android/pylib/jun...
build/android/pylib/junit/test_runner.py:50: if not
os.path.isdir(self._coverage_dir):
On 2016/05/24 19:33:21, jbudorick wrote:
> nit: os.path.exists
> 
> although I'm not sure how either would work if coverage_dir exists but isn't a
> directory

I tested os.path.exists and os.path.isdir.

>>> os.path.exists('/tmp/coverage/kidding.ec')
True
>>> os.path.isdir('/tmp/coverage/kidding.ec')
False
>>> os.path.isdir('/tmp/coverage/')
True

I guess here it should be isdir right?

[jbudorick, 2016-05-25 13:11:37]

https://codereview.chromium.org/2003213002/diff/40001/build/android/pylib/jun...
File build/android/pylib/junit/test_runner.py (right):

https://codereview.chromium.org/2003213002/diff/40001/build/android/pylib/jun...
build/android/pylib/junit/test_runner.py:50: if not
os.path.isdir(self._coverage_dir):
On 2016/05/25 02:00:20, BigBossZhiling wrote:
> On 2016/05/24 19:33:21, jbudorick wrote:
> > nit: os.path.exists
> > 
> > although I'm not sure how either would work if coverage_dir exists but isn't
a
> > directory
> 
> I tested os.path.exists and os.path.isdir.
> 
> >>> os.path.exists('/tmp/coverage/kidding.ec')
> True
> >>> os.path.isdir('/tmp/coverage/kidding.ec')
> False
> >>> os.path.isdir('/tmp/coverage/')
> True

Hah, yes, I know how they work in that sense.

> 
> I guess here it should be isdir right?

To elaborate a bit:
 - I'm not sure how makedirs would react if self._coverage_dir exists but isn't
a directory. I suspect it'll throw an Exception of some kind. Your current
implementation will try that.
 - I'm not sure how emma would react if self._coverage_dir exists but isn't a
directory. I suspect it'll die, but again, I'm not sure.

This should probably do:

  if not os.path.exists(self._coverage_dir):
    os.makedirs(self._coverage_dir)
  elif not os.path.isdir(self._coverage_dir):
    # raise an Exception of some type
  jvm_args.append(...)

[BigBossZhiling, 2016-05-25 17:07:18]

The CQ bit was checked by hzl@google.com

[BigBossZhiling, 2016-05-25 17:07:19]

The patchset sent to the CQ was uploaded after l-g-t-m from
jbudorick@chromium.org
Link to the patchset: https://codereview.chromium.org/2003213002/#ps60001
(title: "fixes")

[commit-bot: I haz the power, 2016-05-25 17:07:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2003213002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2003213002/60001

[commit-bot: I haz the power, 2016-05-25 18:20:23]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-05-25 18:21:45]

Description was changed from

==========
Made junit coverage-dir take directory, not file.

--coverage-dir option for junit tests should take directory name, not file name.
With the change, we are able to run the command like this:
'out-gn/Debug/bin/run_clank_junit_tests --coverage-dir /tmp/coverage/junit'.
Then clank_junit_test.ec will be created in the junit directory.

BUG=
==========

to

==========
Made junit coverage-dir take directory, not file.

--coverage-dir option for junit tests should take directory name, not file name.
With the change, we are able to run the command like this:
'out-gn/Debug/bin/run_clank_junit_tests --coverage-dir /tmp/coverage/junit'.
Then clank_junit_test.ec will be created in the junit directory.

BUG=

Committed: https://crrev.com/f9b5e71a566772a1a80d3e670ed584f60bce71df
Cr-Commit-Position: refs/heads/master@{#395936}
==========

[commit-bot: I haz the power, 2016-05-25 18:21:46]

Patchset 4 (id:??) landed as
https://crrev.com/f9b5e71a566772a1a80d3e670ed584f60bce71df
Cr-Commit-Position: refs/heads/master@{#395936}