Rename upload_gtest_test_results.py to upload_test_results.py.

Rename upload_gtest_test_results.py to upload_test_results.py.

The motivation is to reuse the logic of uploading test results to support other
format such as JSON Test Result format.

*This CL was split from https://crrev.com/2410613002


BUG=649762
Committed: https://chromium.googlesource.com/chromium/tools/build/+/8151d3c3ab517ed5c99b104b00d2a3fd549cc813

[nednguyen, 2016-10-11 15:15:13]

nednguyen@google.com changed reviewers:
+ dpranke@chromium.org, kbr@chromium.org, phajdan.jr@chromium.org,
sergiyb@chromium.org

[nednguyen, 2016-10-11 15:15:31]

PTAL

[Paweł Hajdan Jr., 2016-10-11 16:36:26]

https://codereview.chromium.org/2411763002/diff/40001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/test_results/api.py (right):

https://codereview.chromium.org/2411763002/diff/40001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/test_results/api.py:11: def upload(self,
results_file, test_type, chrome_revision,
It seems useful to have explicit way of knowing type of results we're uploading.

I'm generally skeptical about possible auto-detection (is that the plan?).

What's the advantage otherwise? What code would be shared? Why not extract
shared parts to a module?

[nednguyen, 2016-10-11 17:12:31]

On 2016/10/11 16:36:26, Paweł Hajdan Jr. wrote:
>
https://codereview.chromium.org/2411763002/diff/40001/scripts/slave/recipe_mo...
> File scripts/slave/recipe_modules/test_results/api.py (right):
> 
>
https://codereview.chromium.org/2411763002/diff/40001/scripts/slave/recipe_mo...
> scripts/slave/recipe_modules/test_results/api.py:11: def upload(self,
> results_file, test_type, chrome_revision,
> It seems useful to have explicit way of knowing type of results we're
uploading.

This could be solved by adding logging?

> 
> I'm generally skeptical about possible auto-detection (is that the plan?).

Yes.

> 
> What's the advantage otherwise? What code would be shared? Why not extract
> shared parts to a module?

I can also do this, but it's unclear why splitting the handling the result at
recipe_modules/test_results/api.py level is better than at
test_results/resources/upload_test_results.py level. In fact the former make it
easier to write python unittest for checking the auto-detection logic.

[Ken Russell (switch to Gerrit), 2016-10-11 22:47:43]

LGTM for what it's worth. I think auto-detection of the upload format is fine,
but I'm not an OWNER.

[Sergiy Byelozyorov, 2016-10-12 18:57:20]

lgtm

[Sergiy Byelozyorov, 2016-10-12 18:59:19]

On 2016/10/12 18:57:20, Sergiy Byelozyorov wrote:
> lgtm

This CL was split from https://crrev.com/2410613002

[nednguyen, 2016-10-12 19:03:33]

Description was changed from

==========
Rename upload_gtest_test_results.py to upload_test_results.py.

The motivation is to reuse the logic of uploading test results to support other
format such as JSON Test Result format.

BUG=649762
==========

to

==========
Rename upload_gtest_test_results.py to upload_test_results.py.

The motivation is to reuse the logic of uploading test results to support other
format such as JSON Test Result format.

*This CL was split from https://crrev.com/2410613002


BUG=649762
==========

[nednguyen, 2016-10-12 19:03:40]

The CQ bit was checked by nednguyen@google.com

[commit-bot: I haz the power, 2016-10-12 19:03:44]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-12 19:08:44]

Description was changed from

==========
Rename upload_gtest_test_results.py to upload_test_results.py.

The motivation is to reuse the logic of uploading test results to support other
format such as JSON Test Result format.

*This CL was split from https://crrev.com/2410613002


BUG=649762
==========

to

==========
Rename upload_gtest_test_results.py to upload_test_results.py.

The motivation is to reuse the logic of uploading test results to support other
format such as JSON Test Result format.

*This CL was split from https://crrev.com/2410613002


BUG=649762

Committed:
https://chromium.googlesource.com/chromium/tools/build/+/8151d3c3ab517ed5c99b...
==========

[commit-bot: I haz the power, 2016-10-12 19:08:46]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/tools/build/+/8151d3c3ab517ed5c99b...

[ghost stip (do not use), 2016-10-13 07:49:20]

stip@chromium.org changed reviewers:
+ stip@chromium.org

[ghost stip (do not use), 2016-10-13 07:49:21]

https://codereview.chromium.org/2411763002/diff/40001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/test_results/api.py (right):

https://codereview.chromium.org/2411763002/diff/40001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/test_results/api.py:31: args=['--input-json',
results_file,
this broke test uploading for android. should we revert?

[estaab, 2016-10-13 15:46:38]

estaab@chromium.org changed reviewers:
+ estaab@chromium.org

[estaab, 2016-10-13 15:46:40]

https://codereview.chromium.org/2411763002/diff/40001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/test_results/api.py (right):

https://codereview.chromium.org/2411763002/diff/40001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/test_results/api.py:11: def upload(self,
results_file, test_type, chrome_revision,
On 2016/10/11 at 16:36:26, Paweł Hajdan Jr. wrote:
> It seems useful to have explicit way of knowing type of results we're
uploading.
> 
> I'm generally skeptical about possible auto-detection (is that the plan?).
> 
> What's the advantage otherwise? What code would be shared? Why not extract
shared parts to a module?

I agree with Paweł, this should have been explicit. Callers should be conscious
of the distinction since the behavior is very different for the different use
cases (converting formats vs uploading raw).

[nednguyen, 2016-10-13 15:54:35]

https://codereview.chromium.org/2411763002/diff/40001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/test_results/api.py (right):

https://codereview.chromium.org/2411763002/diff/40001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/test_results/api.py:11: def upload(self,
results_file, test_type, chrome_revision,
On 2016/10/13 15:46:39, estaab wrote:
> On 2016/10/11 at 16:36:26, Paweł Hajdan Jr. wrote:
> > It seems useful to have explicit way of knowing type of results we're
> uploading.
> > 
> > I'm generally skeptical about possible auto-detection (is that the plan?).
> > 
> > What's the advantage otherwise? What code would be shared? Why not extract
> shared parts to a module?
> 
> I agree with Paweł, this should have been explicit. Callers should be
conscious
> of the distinction since the behavior is very different for the different use
> cases (converting formats vs uploading raw).

I don't find how splitting the uploader into 2 scripts: one convert format +
upload and another one only uploading will make it simpler.

I think the bug is the uploader script is doing too much here: both converting
format & uploading. 

If you want, I can move the format converting logic out of
upload_test_results.py. Maybe move the formatting logic to test_results/api.py,
wdyt?

[estaab, 2016-10-20 04:56:42]

https://codereview.chromium.org/2411763002/diff/40001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/test_results/api.py (right):

https://codereview.chromium.org/2411763002/diff/40001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/test_results/api.py:11: def upload(self,
results_file, test_type, chrome_revision,
On 2016/10/13 at 15:54:35, nednguyen wrote:
> On 2016/10/13 15:46:39, estaab wrote:
> > On 2016/10/11 at 16:36:26, Paweł Hajdan Jr. wrote:
> > > It seems useful to have explicit way of knowing type of results we're
> > uploading.
> > > 
> > > I'm generally skeptical about possible auto-detection (is that the plan?).
> > > 
> > > What's the advantage otherwise? What code would be shared? Why not extract
> > shared parts to a module?
> > 
> > I agree with Paweł, this should have been explicit. Callers should be
conscious
> > of the distinction since the behavior is very different for the different
use
> > cases (converting formats vs uploading raw).
> 
> I don't find how splitting the uploader into 2 scripts: one convert format +
upload and another one only uploading will make it simpler.
> 
> I think the bug is the uploader script is doing too much here: both converting
format & uploading. 
> 
> If you want, I can move the format converting logic out of
upload_test_results.py. Maybe move the formatting logic to test_results/api.py,
wdyt?

Sorry, I didn't mean that we need multiple scripts. I would be happy with 2
optional parameters where users set one or the other to upload() and to
upload_test_results.py: a gtest_json and a test_results_json (--gtest-input-json
and --test-results-input-json).

I think it's not worth spending more than 30 minutes changing it at this point
since it doesn't matter that much. The logging you mentioned sounds like a good
compromise and printing "parsed as the gtest json format" or something along
those lines would speed up debugging.