Issue 2439213002: Add step to process Render Test results.

mikecase (-- gone --)

mikecase@chromium.org changed reviewers: + jbudorick@chromium.org, peconn@chromium.org

4 years, 2 months ago (2016-10-21 20:58:02 UTC) #1

mikecase (-- gone --)

Will commit this after commit... https://codereview.chromium.org/2406593002 Also, in that CL, I think Ill have to ...

4 years, 2 months ago (2016-10-21 20:58:03 UTC) #2

PEConn

It doesn't look like you're doing anything crazy, but I'm very unfamiliar with this code. ...

4 years, 1 month ago (2016-10-25 08:31:31 UTC) #3

jbudorick

https://codereview.chromium.org/2439213002/diff/20001/scripts/slave/recipe_modules/chromium_android/api.py File scripts/slave/recipe_modules/chromium_android/api.py (right): https://codereview.chromium.org/2439213002/diff/20001/scripts/slave/recipe_modules/chromium_android/api.py#newcode1544 scripts/slave/recipe_modules/chromium_android/api.py:1544: step_result.presentation.logs[ nit: don't break here; do something like step_result.presentation.logs['render ...

4 years, 1 month ago (2016-10-25 17:03:40 UTC) #5

mikecase (-- gone --)

https://codereview.chromium.org/2439213002/diff/20001/scripts/slave/recipe_modules/chromium_android/api.py File scripts/slave/recipe_modules/chromium_android/api.py (right): https://codereview.chromium.org/2439213002/diff/20001/scripts/slave/recipe_modules/chromium_android/api.py#newcode1544 scripts/slave/recipe_modules/chromium_android/api.py:1544: step_result.presentation.logs[ On 2016/10/25 at 17:03:40, jbudorick wrote: > nit: ...

4 years, 1 month ago (2016-10-25 20:53:34 UTC) #6

https://codereview.chromium.org/2439213002/diff/20001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/chromium_android/api.py (right):

https://codereview.chromium.org/2439213002/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromium_android/api.py:1544:
step_result.presentation.logs[
On 2016/10/25 at 17:03:40, jbudorick wrote:
> nit: don't break here; do something like
> 
>   step_result.presentation.logs['render results'] = (
>       step_result.raw_io.output.splitlines())

Done

https://codereview.chromium.org/2439213002/diff/20001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/chromium_tests/chromium_fyi.py (right):

https://codereview.chromium.org/2439213002/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromium_tests/chromium_fyi.py:1830:
steps.UploadRenderTestFailures(render_results_dir='chrome/test/data/android/render_tests'),
On 2016/10/25 at 17:03:40, jbudorick wrote:
> :O
> 
> Does this need to be a separately configured test? Can we do it some other
way?

hmmmm, well, I could add an argument to run_instrumentation_test() function in
chromium_android/api.py. Done. This seems a bit better.

https://codereview.chromium.org/2439213002/diff/60001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/chromium_android/api.py (right):

https://codereview.chromium.org/2439213002/diff/60001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromium_android/api.py:977:
self.m.step.active_result)
So, idk what you think about this. But if render_results_dir is passed, and
results_details=False, then if the bot also uses the code in steps.py to run
their tests, the results of the test run will get printed to the "[Render Tests]
Process render tests" step.

I think* I know a work-around to this. Which would involve either (1) Using a
nested step in steps.py and printing the results to the outer nested step. Or
(2) move some of the logic done in steps.py into this function.

Or, I could change this CL like how it was in the initial patch and move
upload_render_test_failures out of this function.

jbudorick

https://codereview.chromium.org/2439213002/diff/60001/scripts/slave/recipe_modules/chromium_android/api.py File scripts/slave/recipe_modules/chromium_android/api.py (right): https://codereview.chromium.org/2439213002/diff/60001/scripts/slave/recipe_modules/chromium_android/api.py#newcode977 scripts/slave/recipe_modules/chromium_android/api.py:977: self.m.step.active_result) On 2016/10/25 20:53:34, mikecase wrote: > So, idk ...

4 years, 1 month ago (2016-10-25 21:37:32 UTC) #7

mikecase (-- gone --)

This is what the nested step solution would look like. This works because recipes allows ...

4 years, 1 month ago (2016-10-26 17:31:33 UTC) #8

mikecase (-- gone --)

https://codereview.chromium.org/2439213002/diff/80001/scripts/slave/recipes/chromium.expected/full_chromium_fyi_Android_Remoting_Tests.json File scripts/slave/recipes/chromium.expected/full_chromium_fyi_Android_Remoting_Tests.json (left): https://codereview.chromium.org/2439213002/diff/80001/scripts/slave/recipes/chromium.expected/full_chromium_fyi_Android_Remoting_Tests.json#oldcode798 scripts/slave/recipes/chromium.expected/full_chromium_fyi_Android_Remoting_Tests.json:798: "reason": "1 tests failed: ['ChromotingTest']", oops, I didnt see ...

4 years, 1 month ago (2016-10-26 17:33:02 UTC) #9

mikecase (-- gone --)

https://codereview.chromium.org/2439213002/diff/80001/scripts/slave/recipes/chromium.expected/full_chromium_fyi_Android_Remoting_Tests.json File scripts/slave/recipes/chromium.expected/full_chromium_fyi_Android_Remoting_Tests.json (left): https://codereview.chromium.org/2439213002/diff/80001/scripts/slave/recipes/chromium.expected/full_chromium_fyi_Android_Remoting_Tests.json#oldcode798 scripts/slave/recipes/chromium.expected/full_chromium_fyi_Android_Remoting_Tests.json:798: "reason": "1 tests failed: ['ChromotingTest']", On 2016/10/26 at 17:33:01, ...

4 years, 1 month ago (2016-10-26 17:37:05 UTC) #10

mikecase (-- gone --)

mikecase@chromium.org changed reviewers: + rnephew@chromium.org

4 years, 1 month ago (2016-10-27 22:44:45 UTC) #11

mikecase (-- gone --)

+rnephew since jbudorick is out tomorrow. Would be nice to get this up and running ...

4 years, 1 month ago (2016-10-27 22:44:46 UTC) #12

mikecase (-- gone --)

friendly ping for John. I took another look over the expectation files, and everything seems ...

4 years, 1 month ago (2016-11-03 16:38:13 UTC) #13

jbudorick

https://codereview.chromium.org/2439213002/diff/120001/scripts/slave/recipe_modules/chromium_android/api.py File scripts/slave/recipe_modules/chromium_android/api.py (left): https://codereview.chromium.org/2439213002/diff/120001/scripts/slave/recipe_modules/chromium_android/api.py#oldcode970 scripts/slave/recipe_modules/chromium_android/api.py:970: self.copy_gtest_results(result_step, This worries me. https://codereview.chromium.org/2439213002/diff/120001/scripts/slave/recipe_modules/chromium_tests/chromium_fyi.py File scripts/slave/recipe_modules/chromium_tests/chromium_fyi.py (right): https://codereview.chromium.org/2439213002/diff/120001/scripts/slave/recipe_modules/chromium_tests/chromium_fyi.py#newcode1829 ...

4 years, 1 month ago (2016-11-03 18:34:36 UTC) #14

mikecase (-- gone --)

https://codereview.chromium.org/2439213002/diff/120001/scripts/slave/recipe_modules/chromium_android/api.py File scripts/slave/recipe_modules/chromium_android/api.py (left): https://codereview.chromium.org/2439213002/diff/120001/scripts/slave/recipe_modules/chromium_android/api.py#oldcode970 scripts/slave/recipe_modules/chromium_android/api.py:970: self.copy_gtest_results(result_step, On 2016/11/03 at 18:34:36, jbudorick wrote: > This ...

4 years, 1 month ago (2016-11-03 18:41:37 UTC) #15

mikecase (-- gone --)

https://codereview.chromium.org/2439213002/diff/120001/scripts/slave/recipe_modules/chromium_tests/steps.py File scripts/slave/recipe_modules/chromium_tests/steps.py (right): https://codereview.chromium.org/2439213002/diff/120001/scripts/slave/recipe_modules/chromium_tests/steps.py#newcode1508 scripts/slave/recipe_modules/chromium_tests/steps.py:1508: api.test_utils.format_step_text([['failures:', failures]])) This is basically the only important line ...

4 years, 1 month ago (2016-11-03 18:56:33 UTC) #16

jbudorick

https://codereview.chromium.org/2439213002/diff/120001/scripts/slave/recipe_modules/chromium_android/api.py File scripts/slave/recipe_modules/chromium_android/api.py (left): https://codereview.chromium.org/2439213002/diff/120001/scripts/slave/recipe_modules/chromium_android/api.py#oldcode970 scripts/slave/recipe_modules/chromium_android/api.py:970: self.copy_gtest_results(result_step, On 2016/11/03 18:41:37, mikecase wrote: > On 2016/11/03 ...

4 years, 1 month ago (2016-11-07 21:58:57 UTC) #18

https://codereview.chromium.org/2439213002/diff/120001/scripts/slave/recipe_m...
File scripts/slave/recipe_modules/chromium_android/api.py (left):

https://codereview.chromium.org/2439213002/diff/120001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/chromium_android/api.py:970:
self.copy_gtest_results(result_step,
On 2016/11/03 18:41:37, mikecase wrote:
> On 2016/11/03 at 18:34:36, jbudorick wrote:
> > This worries me.
> 
> I think this should be fine. As long as nothing tries to alter the step_result
> that is returned other than in steps.py (which I fix in this CL).

I'm particularly worried if something tries to use the gtest results.

https://codereview.chromium.org/2439213002/diff/120001/scripts/slave/recipe_m...
File scripts/slave/recipe_modules/chromium_tests/chromium_fyi.py (right):

https://codereview.chromium.org/2439213002/diff/120001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/chromium_tests/chromium_fyi.py:1829:
steps.AndroidInstrumentationTest(
On 2016/11/03 18:41:37, mikecase wrote:
> On 2016/11/03 at 18:34:36, jbudorick wrote:
> > We should be moving away from tests configured here, not towards them.
> 
> Where should I configure this?

src/testing/buildbot/chromium.fyi.json

https://codereview.chromium.org/2439213002/diff/120001/scripts/slave/recipe_m...
File scripts/slave/recipe_modules/chromium_tests/steps.py (right):

https://codereview.chromium.org/2439213002/diff/120001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/chromium_tests/steps.py:1487: def run(self, api,
suffix, test_filter=None):
On 2016/11/03 18:41:37, mikecase wrote:
> On 2016/11/03 at 18:34:36, jbudorick wrote:
> > Implementing this in here will not work on the bots that use swarming --
i.e.,
> the trybots or the main waterfall.
> 
> I don't think that is a problem. The logic to run the render test stuff is in
> chromium_android/api.py. These changes are just to adapt to me removing the
> copy_gtest_results function.

I mean, it's ok for now, but if you want to use this on the try bots, it'll need
to be usable on swarming.

(The same applies to the results processing logic, though.)

https://codereview.chromium.org/2439213002/diff/120001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/chromium_tests/steps.py:1508:
api.test_utils.format_step_text([['failures:', failures]]))
On 2016/11/03 18:56:33, mikecase wrote:
> This is basically the only important line I added to this file.
> 
> Before
> 
> - run_instrumentation_test() function copies test results from the test_runner
> step to the last step that was run. This might not be the test_runner step and
> could be anything which is kinda awkward
> - This function grabs test results from the active_result (last step that was
> run). It then prints out the results to the active_result's step text (again,
> this step might not be the test_runner step so the results could get printed
> somewhere awkward and inconsistent)
> 
> Now
> 
> - run_instrumentation_test() function DOES NOT copy test results to last step.
> Instead, it explicitly returns the test runner step.
> - This function grabs the test results from the test_runner step. However, it
> might not be able to modify that step's step_text because it is not guaranteed
> to be the last step to have run. Instead, it print the results to the new
outer
> nested_step I added.
> 
> Alternatively, we could have run_instrumentation_tests() function print the
> results to the step_text which might work

Ah, yeah, I see what you mean now.

mikecase (-- gone --)

https://codereview.chromium.org/2439213002/diff/120001/scripts/slave/recipe_modules/chromium_android/api.py File scripts/slave/recipe_modules/chromium_android/api.py (left): https://codereview.chromium.org/2439213002/diff/120001/scripts/slave/recipe_modules/chromium_android/api.py#oldcode970 scripts/slave/recipe_modules/chromium_android/api.py:970: self.copy_gtest_results(result_step, On 2016/11/07 at 21:58:56, jbudorick wrote: > On ...

4 years, 1 month ago (2016-11-07 23:38:29 UTC) #19

mikecase (-- gone --)

mikecase@chromium.org changed reviewers: + yolandyan@chromium.org - rnephew@chromium.org

4 years, 1 month ago (2016-11-10 22:51:16 UTC) #20

PEConn

On 2016/11/10 22:51:17, mikecase wrote: > +yoland for review Is there anything blocking this beyond ...

4 years, 1 month ago (2016-11-21 17:58:03 UTC) #22

mikecase (-- gone --)

On 2016/11/21 at 17:58:03, peconn wrote: > On 2016/11/10 22:51:17, mikecase wrote: > > +yoland ...

4 years, 1 month ago (2016-11-21 17:58:58 UTC) #23

jbudorick

Add a test to recipes/chromium.py that sets the render results dir. https://codereview.chromium.org/2439213002/diff/140001/scripts/slave/recipe_modules/chromium_tests/steps.py File scripts/slave/recipe_modules/chromium_tests/steps.py (right): ...

4 years, 1 month ago (2016-11-21 18:08:50 UTC) #24

mikecase (-- gone --)

mikecase@chromium.org changed reviewers: + stip@chromium.org - yolandyan@chromium.org

4 years, 1 month ago (2016-11-21 23:03:07 UTC) #25

mikecase (-- gone --)

Added test for chromium.py. Adding stip@ since I need review for chromium_tests/steps.py I think https://codereview.chromium.org/2439213002/diff/140001/scripts/slave/recipe_modules/chromium_tests/steps.py ...

4 years, 1 month ago (2016-11-21 23:03:12 UTC) #26

ghost stip (do not use)

whoa, this is going to take a bit of review on my part. I'm concerned ...

4 years, 1 month ago (2016-11-22 08:12:24 UTC) #27

jbudorick

On 2016/12/05 19:38:31, mikecase wrote: > ping. PTAL :D Given stip's last comment, I'm letting ...

4 years ago (2016-12-06 14:50:55 UTC) #31

ghost stip (do not use)

hey, sorry for the mega-delay here. I think this patch is generally okay and can ...

4 years ago (2016-12-08 20:16:02 UTC) #32

mikecase (-- gone --)

https://codereview.chromium.org/2439213002/diff/160001/scripts/slave/recipe_modules/chromium_android/example.py File scripts/slave/recipe_modules/chromium_android/example.py (right): https://codereview.chromium.org/2439213002/diff/160001/scripts/slave/recipe_modules/chromium_android/example.py#newcode233 scripts/slave/recipe_modules/chromium_android/example.py:233: render_results_dir=config.get('render_results_dir')) On 2016/12/08 at 20:16:02, stip wrote: > ideally, ...

3 years, 11 months ago (2017-01-09 22:27:09 UTC) #33

mikecase (-- gone --)

The CQ bit was checked by mikecase@chromium.org

3 years, 11 months ago (2017-01-09 23:19:24 UTC) #34

mikecase (-- gone --)

The patchset sent to the CQ was uploaded after l-g-t-m from stip@chromium.org Link to the ...

3 years, 11 months ago (2017-01-09 23:19:24 UTC) #35

commit-bot: I haz the power

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

3 years, 11 months ago (2017-01-09 23:19:29 UTC) #36

mikecase (-- gone --)

Any idea if I have to reupload CL due to the code review tool changing ...

3 years, 11 months ago (2017-01-09 23:22:50 UTC) #37

commit-bot: I haz the power

CQ is committing da patch. Bot data: {"patchset_id": 180001, "attempt_start_ts": 1484003964166540, "parent_rev": "8a98359c20421c6316fc05bf0fb89f1914f49ee1", "commit_rev": "68ae7b2f41e9c5eb9a57cfa7814a646e38ab3d20"}

3 years, 11 months ago (2017-01-09 23:24:29 UTC) #38

commit-bot: I haz the power

Description was changed from ========== Add step to process Render Test results. Add new steps ...

3 years, 11 months ago (2017-01-09 23:24:32 UTC) #39

commit-bot: I haz the power

3 years, 11 months ago (2017-01-09 23:24:33 UTC) #40

Message was sent while issue was closed.

Committed patchset #10 (id:180001) as
https://chromium.googlesource.com/chromium/tools/build/+/68ae7b2f41e9c5eb9a57...

Issue 2439213002: Add step to process Render Test results. (Closed)

Description

Patch Set 1 #

Patch Set 2 : Add step to process Render Test results. #

Patch Set 3 : Removed UploadRenderTestFailures Test. Added arg to run_instrumentation_suite. #

Patch Set 4 : Add step to process Render Test results. #

Patch Set 5 : Add results to nested ste. #

Patch Set 6 : Fixed not re-raising test failures #

Patch Set 7 : Added parens to nested_step_name #

Patch Set 8 : Add step to process Render Test results. #

Patch Set 9 : Addressed jbudorick's nits #

Patch Set 10 : Rebase #

Messages