Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2063323002:
Create recipe to run find_annotated_tests.py periodically (Closed)
|
Created:
4 years, 6 months ago by the real yoland Modified:
4 years, 5 months ago CC:
chromium-reviews, infra-reviews+build_chromium.org, kjellander-cc_chromium.org Base URL:
https://chromium.googlesource.com/chromium/tools/build.git@master Target Ref:
refs/heads/master Project:
build Visibility:
Public. |
DescriptionCreate recipe to run find_annotated_tests.py periodically
BUG=601909
Committed: https://chromium.googlesource.com/chromium/tools/build/+/b8aaec337558e95483a4b7050cb18ac2170ff518
Patch Set 1 #Patch Set 2 : Add mastername, buildername to GenTest #
Total comments: 2
Patch Set 3 : Name mistake #
Total comments: 12
Patch Set 4 : Compile the targets #Patch Set 5 : Change after mike's comments #
Total comments: 21
Patch Set 6 : Change after both reviewers' comments #Patch Set 7 : Change after both reviewers' comments #Patch Set 8 : Add into steps.py #Patch Set 9 : Move things to a step #Patch Set 10 : Move from recipe to a step #
Total comments: 7
Patch Set 11 : Use context manager to temp dir #Patch Set 12 : No find copies #Patch Set 13 : Rebase #Patch Set 14 : Set current_time property for gen tests #Patch Set 15 : Pass branch info to Nightly scheduler #
Messages
Total messages: 37 (9 generated)
yolandyan@google.com changed reviewers: + jbudorick@chromium.org, mikecase@chromium.org, yolandyan@google.com
This CL is now ready for review
This CL is now ready for review
https://codereview.chromium.org/2063323002/diff/20001/scripts/slave/recipes/f... File scripts/slave/recipes/find_annotated_tests.py (right): https://codereview.chromium.org/2063323002/diff/20001/scripts/slave/recipes/f... scripts/slave/recipes/find_annotated_tests.py:50: '--script_runtime_string', script_runtime_string, nit: Chnage to same format as everything else. script_runtime_string to script-runtime-string https://codereview.chromium.org/2063323002/diff/40001/scripts/slave/recipes/f... File scripts/slave/recipes/find_annotated_tests.py (right): https://codereview.chromium.org/2063323002/diff/40001/scripts/slave/recipes/f... scripts/slave/recipes/find_annotated_tests.py:39: api.gclient.checkout() As discussed, may need to add a compile step. https://codereview.chromium.org/2063323002/diff/40001/scripts/slave/recipes/f... scripts/slave/recipes/find_annotated_tests.py:40: temp_output_dir = api.path.mkdtemp(JSON_OUTPUT_DIR) May want this in a try block, and the delete rmtree step in a finally block. https://codereview.chromium.org/2063323002/diff/40001/scripts/slave/recipes/f... scripts/slave/recipes/find_annotated_tests.py:47: '--test-apks', ' '.join(TEST_APKS), you probably have to pass the full path of the apk Im guessing and not just the name of the file? Unless that is what you use --apk-output-dir for??? https://codereview.chromium.org/2063323002/diff/40001/scripts/slave/recipes/f... scripts/slave/recipes/find_annotated_tests.py:50: '--script_runtime_string', script_runtime_string, Why can't you have an --output-file-name or something, and then just rename the file to have the runtime_string when you upload.
https://codereview.chromium.org/2063323002/diff/20001/scripts/slave/recipes/f... File scripts/slave/recipes/find_annotated_tests.py (right): https://codereview.chromium.org/2063323002/diff/20001/scripts/slave/recipes/f... scripts/slave/recipes/find_annotated_tests.py:50: '--script_runtime_string', script_runtime_string, On 2016/06/14 at 22:22:50, mikecase wrote: > nit: Chnage to same format as everything else. > > script_runtime_string to script-runtime-string Done https://codereview.chromium.org/2063323002/diff/40001/scripts/slave/recipes/f... File scripts/slave/recipes/find_annotated_tests.py (right): https://codereview.chromium.org/2063323002/diff/40001/scripts/slave/recipes/f... scripts/slave/recipes/find_annotated_tests.py:39: api.gclient.checkout() On 2016/06/14 at 22:22:51, mikecase wrote: > As discussed, may need to add a compile step. Done https://codereview.chromium.org/2063323002/diff/40001/scripts/slave/recipes/f... scripts/slave/recipes/find_annotated_tests.py:40: temp_output_dir = api.path.mkdtemp(JSON_OUTPUT_DIR) On 2016/06/14 at 22:22:51, mikecase wrote: > May want this in a try block, and the delete rmtree step in a finally block. Done https://codereview.chromium.org/2063323002/diff/40001/scripts/slave/recipes/f... scripts/slave/recipes/find_annotated_tests.py:47: '--test-apks', ' '.join(TEST_APKS), On 2016/06/14 at 22:22:51, mikecase wrote: > you probably have to pass the full path of the apk Im guessing and not just the name of the file? Unless that is what you use --apk-output-dir for??? yes....the jar files are like `out/Debug/test.lib.java/ChromeTest.jar` so I pass in out/Debug part and the jar name... DING DING DING? https://codereview.chromium.org/2063323002/diff/40001/scripts/slave/recipes/f... scripts/slave/recipes/find_annotated_tests.py:50: '--script_runtime_string', script_runtime_string, On 2016/06/14 at 22:22:51, mikecase wrote: > Why can't you have an --output-file-name or something, and then just rename the file to have the runtime_string when you upload. Because I want to add the script runtime IN to the json file as well. Either that, or the app engine will parse the file name's runtime info and store it in its database What do you think?
https://codereview.chromium.org/2063323002/diff/40001/scripts/slave/recipes/f... File scripts/slave/recipes/find_annotated_tests.py (right): https://codereview.chromium.org/2063323002/diff/40001/scripts/slave/recipes/f... scripts/slave/recipes/find_annotated_tests.py:47: '--test-apks', ' '.join(TEST_APKS), On 2016/06/14 at 23:27:15, the real yoland wrote: > On 2016/06/14 at 22:22:51, mikecase wrote: > > you probably have to pass the full path of the apk Im guessing and not just the name of the file? Unless that is what you use --apk-output-dir for??? > > yes....the jar files are like `out/Debug/test.lib.java/ChromeTest.jar` so I pass in out/Debug part and the jar name... > DING DING DING? Lol. I think it would be best to pass in full paths to the apk and a full path to the jars you need (unless doing this is unreasonably messy) https://codereview.chromium.org/2063323002/diff/40001/scripts/slave/recipes/f... scripts/slave/recipes/find_annotated_tests.py:50: '--script_runtime_string', script_runtime_string, On 2016/06/14 at 23:27:15, the real yoland wrote: > On 2016/06/14 at 22:22:51, mikecase wrote: > > Why can't you have an --output-file-name or something, and then just rename the file to have the runtime_string when you upload. > > Because I want to add the script runtime IN to the json file as well. Either that, or the app engine will parse the file name's runtime info and store it in its database > What do you think? oh, then this sounds fine to me. Might want to rename it to timestamp-string since runtime-string sounds like it has a different meaning. https://codereview.chromium.org/2063323002/diff/80001/scripts/slave/recipes/f... File scripts/slave/recipes/find_annotated_tests.py (right): https://codereview.chromium.org/2063323002/diff/80001/scripts/slave/recipes/f... scripts/slave/recipes/find_annotated_tests.py:36: 'system_webview_shell_layout_test_apk': 'SystemWebViewShellLayoutTest', I think you want to remove components_browsertests_apk and content_browsertests_apk https://codereview.chromium.org/2063323002/diff/80001/scripts/slave/recipes/f... scripts/slave/recipes/find_annotated_tests.py:37: } nit: I dont think this should be indented. https://codereview.chromium.org/2063323002/diff/80001/scripts/slave/recipes/f... scripts/slave/recipes/find_annotated_tests.py:57: 'tools', 'android', 'find_annotated_tests.py'), nit: I think this needs to be indented 2 more spaces https://codereview.chromium.org/2063323002/diff/80001/scripts/slave/recipes/f... scripts/slave/recipes/find_annotated_tests.py:59: '--test-apks', ' '.join(TEST_APKS.itervalues()), These don't have the .apk extension. Is this what you want? https://codereview.chromium.org/2063323002/diff/80001/scripts/slave/recipes/f... scripts/slave/recipes/find_annotated_tests.py:74: 'find_annotated_tests_basic') + nit: think this should be indented 2 more spaces https://codereview.chromium.org/2063323002/diff/80001/scripts/slave/recipes/f... scripts/slave/recipes/find_annotated_tests.py:76: buildername='Chromium Android Find Annotated Tests', nit: think this should be indented 2 more spaces.
https://codereview.chromium.org/2063323002/diff/40001/scripts/slave/recipes/f... File scripts/slave/recipes/find_annotated_tests.py (right): https://codereview.chromium.org/2063323002/diff/40001/scripts/slave/recipes/f... scripts/slave/recipes/find_annotated_tests.py:47: '--test-apks', ' '.join(TEST_APKS), On 2016/06/14 at 23:43:36, mikecase wrote: > On 2016/06/14 at 23:27:15, the real yoland wrote: > > On 2016/06/14 at 22:22:51, mikecase wrote: > > > you probably have to pass the full path of the apk Im guessing and not just the name of the file? Unless that is what you use --apk-output-dir for??? > > > > yes....the jar files are like `out/Debug/test.lib.java/ChromeTest.jar` so I pass in out/Debug part and the jar name... > > DING DING DING? > > Lol. I think it would be best to pass in full paths to the apk and a full path to the jars you need (unless doing this is unreasonably messy) Hmm, actually because the jars are stored in out/[build]/test.lib.java/[apk_name].jar, it might be bad to hard the test.lib.java in recipe code. The find script get it through constants.SDK_BUILD_TEST_JAVALIB_DIR https://codereview.chromium.org/2063323002/diff/40001/scripts/slave/recipes/f... scripts/slave/recipes/find_annotated_tests.py:50: '--script_runtime_string', script_runtime_string, On 2016/06/14 at 23:43:36, mikecase wrote: > On 2016/06/14 at 23:27:15, the real yoland wrote: > > On 2016/06/14 at 22:22:51, mikecase wrote: > > > Why can't you have an --output-file-name or something, and then just rename the file to have the runtime_string when you upload. > > > > Because I want to add the script runtime IN to the json file as well. Either that, or the app engine will parse the file name's runtime info and store it in its database > > What do you think? > > oh, then this sounds fine to me. Might want to rename it to timestamp-string since runtime-string sounds like it has a different meaning. Done
https://codereview.chromium.org/2063323002/diff/80001/masters/master.chromium... File masters/master.chromium.infra.cron/master.cfg (right): https://codereview.chromium.org/2063323002/diff/80001/masters/master.chromium... masters/master.chromium.infra.cron/master.cfg:159: 'name': 'Chromium Android Find Annotated Test', What's your reasoning behind putting this on chromium.infra.cron? https://codereview.chromium.org/2063323002/diff/80001/scripts/slave/recipes/f... File scripts/slave/recipes/find_annotated_tests.py (right): https://codereview.chromium.org/2063323002/diff/80001/scripts/slave/recipes/f... scripts/slave/recipes/find_annotated_tests.py:1: # Copyright 2016 The Chromium Authors. All rights reserved. This should almost certainly be in android/ given that it is specific to android. https://codereview.chromium.org/2063323002/diff/80001/scripts/slave/recipes/f... scripts/slave/recipes/find_annotated_tests.py:36: 'system_webview_shell_layout_test_apk': 'SystemWebViewShellLayoutTest', On 2016/06/14 23:43:37, mikecase wrote: > I think you want to remove components_browsertests_apk and > content_browsertests_apk Yeah, you're not going to find any java tests in those :) https://codereview.chromium.org/2063323002/diff/80001/scripts/slave/recipes/f... scripts/slave/recipes/find_annotated_tests.py:42: def RunSteps(api): I'm not convinced that this needs to be its own recipe instead of a step in another one, i.e. the chromium recipe.
https://codereview.chromium.org/2063323002/diff/80001/masters/master.chromium... File masters/master.chromium.infra.cron/master.cfg (right): https://codereview.chromium.org/2063323002/diff/80001/masters/master.chromium... masters/master.chromium.infra.cron/master.cfg:159: 'name': 'Chromium Android Find Annotated Test', On 2016/06/20 at 09:28:26, jbudorick (EMEA til June 30) wrote: > What's your reasoning behind putting this on chromium.infra.cron? Because I thought all the timely scheduled jobs belongs to this master (with the word cron) https://codereview.chromium.org/2063323002/diff/80001/scripts/slave/recipes/f... File scripts/slave/recipes/find_annotated_tests.py (right): https://codereview.chromium.org/2063323002/diff/80001/scripts/slave/recipes/f... scripts/slave/recipes/find_annotated_tests.py:1: # Copyright 2016 The Chromium Authors. All rights reserved. On 2016/06/20 at 09:28:26, jbudorick (EMEA til June 30) wrote: > This should almost certainly be in android/ given that it is specific to android. Done https://codereview.chromium.org/2063323002/diff/80001/scripts/slave/recipes/f... scripts/slave/recipes/find_annotated_tests.py:36: 'system_webview_shell_layout_test_apk': 'SystemWebViewShellLayoutTest', On 2016/06/20 at 09:28:26, jbudorick (EMEA til June 30) wrote: > On 2016/06/14 23:43:37, mikecase wrote: > > I think you want to remove components_browsertests_apk and > > content_browsertests_apk > > Yeah, you're not going to find any java tests in those :) Got it! https://codereview.chromium.org/2063323002/diff/80001/scripts/slave/recipes/f... scripts/slave/recipes/find_annotated_tests.py:37: } On 2016/06/14 at 23:43:37, mikecase wrote: > nit: I dont think this should be indented. Done https://codereview.chromium.org/2063323002/diff/80001/scripts/slave/recipes/f... scripts/slave/recipes/find_annotated_tests.py:42: def RunSteps(api): On 2016/06/20 at 09:28:26, jbudorick (EMEA til June 30) wrote: > I'm not convinced that this needs to be its own recipe instead of a step in another one, i.e. the chromium recipe. The problem with that is I only need this to run around 4 times a day. If this becomes a step of another recipe, I will have to time it by estimating how many runs does it take to get to a 6 hours gap. Of course, I can just give undershoot the estimate and upload more jsons than the dashboard needs. The other thing is, it seems rather unclean to have it in another recipe mingled with a bunch of unrelated steps, it will be hard to debug what went wrong. https://codereview.chromium.org/2063323002/diff/80001/scripts/slave/recipes/f... scripts/slave/recipes/find_annotated_tests.py:57: 'tools', 'android', 'find_annotated_tests.py'), On 2016/06/14 at 23:43:37, mikecase wrote: > nit: I think this needs to be indented 2 more spaces Done https://codereview.chromium.org/2063323002/diff/80001/scripts/slave/recipes/f... scripts/slave/recipes/find_annotated_tests.py:59: '--test-apks', ' '.join(TEST_APKS.itervalues()), On 2016/06/14 at 23:43:37, mikecase wrote: > These don't have the .apk extension. Is this what you want? Ya, the script add the extensions https://codereview.chromium.org/2063323002/diff/80001/scripts/slave/recipes/f... scripts/slave/recipes/find_annotated_tests.py:74: 'find_annotated_tests_basic') + On 2016/06/14 at 23:43:37, mikecase wrote: > nit: think this should be indented 2 more spaces Done https://codereview.chromium.org/2063323002/diff/80001/scripts/slave/recipes/f... scripts/slave/recipes/find_annotated_tests.py:76: buildername='Chromium Android Find Annotated Tests', On 2016/06/14 at 23:43:37, mikecase wrote: > nit: think this should be indented 2 more spaces. Done
https://codereview.chromium.org/2063323002/diff/80001/masters/master.chromium... File masters/master.chromium.infra.cron/master.cfg (right): https://codereview.chromium.org/2063323002/diff/80001/masters/master.chromium... masters/master.chromium.infra.cron/master.cfg:159: 'name': 'Chromium Android Find Annotated Test', On 2016/06/21 23:16:08, the real yoland wrote: > On 2016/06/20 at 09:28:26, jbudorick (EMEA til June 30) wrote: > > What's your reasoning behind putting this on chromium.infra.cron? > > Because I thought all the timely scheduled jobs belongs to this master (with the > word cron) No, that's definitely not the case. This is specifically for infra tasks, and I don't think this qualifies. I think a new section in chromium.android is a better fit. https://codereview.chromium.org/2063323002/diff/80001/scripts/slave/recipes/f... File scripts/slave/recipes/find_annotated_tests.py (right): https://codereview.chromium.org/2063323002/diff/80001/scripts/slave/recipes/f... scripts/slave/recipes/find_annotated_tests.py:42: def RunSteps(api): On 2016/06/21 23:16:08, the real yoland wrote: > On 2016/06/20 at 09:28:26, jbudorick (EMEA til June 30) wrote: > > I'm not convinced that this needs to be its own recipe instead of a step in > another one, i.e. the chromium recipe. > > The problem with that is I only need this to run around 4 times a day. If this > becomes a step of another recipe, I will have to time it by estimating how many > runs does it take to get to a 6 hours gap. > Of course, I can just give undershoot the estimate and upload more jsons than > the dashboard needs. > The other thing is, it seems rather unclean to have it in another recipe mingled > with a bunch of unrelated steps, it will be hard to debug what went wrong. To clarify, I'm not saying this shouldn't be it's own bot. It should be. However, I think that this could be a configurable step in the chromium recipe, which does src/ side configuration for many of its steps in //testing/buildbot.
On 2016/06/22 at 10:21:37, jbudorick wrote: > https://codereview.chromium.org/2063323002/diff/80001/masters/master.chromium... > File masters/master.chromium.infra.cron/master.cfg (right): > > https://codereview.chromium.org/2063323002/diff/80001/masters/master.chromium... > masters/master.chromium.infra.cron/master.cfg:159: 'name': 'Chromium Android Find Annotated Test', > On 2016/06/21 23:16:08, the real yoland wrote: > > On 2016/06/20 at 09:28:26, jbudorick (EMEA til June 30) wrote: > > > What's your reasoning behind putting this on chromium.infra.cron? > > > > Because I thought all the timely scheduled jobs belongs to this master (with the > > word cron) > > No, that's definitely not the case. This is specifically for infra tasks, and I don't think this qualifies. I think a new section in chromium.android is a better fit. > > https://codereview.chromium.org/2063323002/diff/80001/scripts/slave/recipes/f... > File scripts/slave/recipes/find_annotated_tests.py (right): > > https://codereview.chromium.org/2063323002/diff/80001/scripts/slave/recipes/f... > scripts/slave/recipes/find_annotated_tests.py:42: def RunSteps(api): > On 2016/06/21 23:16:08, the real yoland wrote: > > On 2016/06/20 at 09:28:26, jbudorick (EMEA til June 30) wrote: > > > I'm not convinced that this needs to be its own recipe instead of a step in > > another one, i.e. the chromium recipe. > > > > The problem with that is I only need this to run around 4 times a day. If this > > becomes a step of another recipe, I will have to time it by estimating how many > > runs does it take to get to a 6 hours gap. > > Of course, I can just give undershoot the estimate and upload more jsons than > > the dashboard needs. > > The other thing is, it seems rather unclean to have it in another recipe mingled > > with a bunch of unrelated steps, it will be hard to debug what went wrong. > > To clarify, I'm not saying this shouldn't be it's own bot. It should be. However, I think that this could be a configurable step in the chromium recipe, which does src/ side configuration for many of its steps in //testing/buildbot. Not sure if you meant this: https://codereview.chromium.org/2063323002/diff/140001/scripts/slave/recipe_m... If so I am really curious on why this should become a step instead just part of its own recipe?
On 2016/06/25 01:14:27, the real yoland wrote: > On 2016/06/22 at 10:21:37, jbudorick wrote: > > > https://codereview.chromium.org/2063323002/diff/80001/masters/master.chromium... > > File masters/master.chromium.infra.cron/master.cfg (right): > > > > > https://codereview.chromium.org/2063323002/diff/80001/masters/master.chromium... > > masters/master.chromium.infra.cron/master.cfg:159: 'name': 'Chromium Android > Find Annotated Test', > > On 2016/06/21 23:16:08, the real yoland wrote: > > > On 2016/06/20 at 09:28:26, jbudorick (EMEA til June 30) wrote: > > > > What's your reasoning behind putting this on chromium.infra.cron? > > > > > > Because I thought all the timely scheduled jobs belongs to this master (with > the > > > word cron) > > > > No, that's definitely not the case. This is specifically for infra tasks, and > I don't think this qualifies. I think a new section in chromium.android is a > better fit. > > > > > https://codereview.chromium.org/2063323002/diff/80001/scripts/slave/recipes/f... > > File scripts/slave/recipes/find_annotated_tests.py (right): > > > > > https://codereview.chromium.org/2063323002/diff/80001/scripts/slave/recipes/f... > > scripts/slave/recipes/find_annotated_tests.py:42: def RunSteps(api): > > On 2016/06/21 23:16:08, the real yoland wrote: > > > On 2016/06/20 at 09:28:26, jbudorick (EMEA til June 30) wrote: > > > > I'm not convinced that this needs to be its own recipe instead of a step > in > > > another one, i.e. the chromium recipe. > > > > > > The problem with that is I only need this to run around 4 times a day. If > this > > > becomes a step of another recipe, I will have to time it by estimating how > many > > > runs does it take to get to a 6 hours gap. > > > Of course, I can just give undershoot the estimate and upload more jsons > than > > > the dashboard needs. > > > The other thing is, it seems rather unclean to have it in another recipe > mingled > > > with a bunch of unrelated steps, it will be hard to debug what went wrong. > > > > To clarify, I'm not saying this shouldn't be it's own bot. It should be. > However, I think that this could be a configurable step in the chromium recipe, > which does src/ side configuration for many of its steps in //testing/buildbot. > > Not sure if you meant this: > https://codereview.chromium.org/2063323002/diff/140001/scripts/slave/recipe_m... > If so I am really curious on why this should become a step instead just part of > its own recipe? Because having this as its own recipe is a higher maintenance burden than having it run in the chromium recipe.
Sorry about the confusion :( https://codereview.chromium.org/2063323002/diff/180001/scripts/slave/recipe_m... File scripts/slave/recipe_modules/chromium_tests/steps.py (right): https://codereview.chromium.org/2063323002/diff/180001/scripts/slave/recipe_m... scripts/slave/recipe_modules/chromium_tests/steps.py:1866: class FindAnnotatedTest(Test): Hm, I should've been more clear. This isn't what I meant, either. I'm instead wondering about the feasibility of implementing this as a ScriptTest that gets specified in src/testing/buildbot/chromium.fyi.json.
On 2016/06/28 at 10:37:19, jbudorick wrote: > Sorry about the confusion :( > > https://codereview.chromium.org/2063323002/diff/180001/scripts/slave/recipe_m... > File scripts/slave/recipe_modules/chromium_tests/steps.py (right): > > https://codereview.chromium.org/2063323002/diff/180001/scripts/slave/recipe_m... > scripts/slave/recipe_modules/chromium_tests/steps.py:1866: class FindAnnotatedTest(Test): > Hm, I should've been more clear. This isn't what I meant, either. I'm instead wondering about the feasibility of implementing this as a ScriptTest that gets specified in src/testing/buildbot/chromium.fyi.json. Hmm, I assume you meant adding a new script in `src/testing/scripts/` so we will be able to just add it in `chromium.fyi.json`. But we will have this script to run find_annotated_tests.py and upload the result in the same step(pretty sure it's a bad idea) or a seperate step(create an extra upload file step in step.py anyway)? It certainly can be done, but I think it just over complicate things new: chromium.py->chromium_fyi.py->testing/buildbot/chromium.fyi.json->src/testing/scripts/run_find_annotated_tests.py->find_annotated_tests.py->steps.UploadResult vs old: chromium.py->chromium_fyi.py->steps.FindAnnotatedTest Or maybe I just didn't get it right?
After talking with Yoland, I'm ok with this implementation. lgtm with some comments You will need an OWNER from recipe_modules/chromium_tests (neither me or John are OWNERS) https://codereview.chromium.org/2063323002/diff/180001/scripts/slave/recipe_m... File scripts/slave/recipe_modules/chromium_tests/steps.py (right): https://codereview.chromium.org/2063323002/diff/180001/scripts/slave/recipe_m... scripts/slave/recipe_modules/chromium_tests/steps.py:1866: class FindAnnotatedTest(Test): On 2016/06/28 at 10:37:19, jbudorick (EMEA til June 30) wrote: > Hm, I should've been more clear. This isn't what I meant, either. I'm instead wondering about the feasibility of implementing this as a ScriptTest that gets specified in src/testing/buildbot/chromium.fyi.json. After talking with Yoland it seems like making it a script test isnt the best idea. This needs to upload some things to Google Storage and clean up temp files after the script runs. Moving these things into a script would duplicate code (we already have recipe APIs to do these things) and just make it more complex. https://codereview.chromium.org/2063323002/diff/180001/scripts/slave/recipe_m... scripts/slave/recipe_modules/chromium_tests/steps.py:1867: test_apks = { eh, _TEST_APKS probably. or _DEFAULT_TEST_APKS https://codereview.chromium.org/2063323002/diff/180001/scripts/slave/recipe_m... scripts/slave/recipe_modules/chromium_tests/steps.py:1891: '--test-apks', ' '.join(FindAnnotatedTest.test_apks.values()), How does your script find the test apks? Should you be passing the fullpaths to the apks to this script?
On 2016/06/28 22:41:32, mikecase wrote: > After talking with Yoland, I'm ok with this implementation. > > lgtm with some comments > > You will need an OWNER from recipe_modules/chromium_tests (neither me or John > are OWNERS) While I disagree with this approach & would prefer to see this as a src/ side script, I'll defer to the chromium_tests owners. If they're ok with this, so am I. > > https://codereview.chromium.org/2063323002/diff/180001/scripts/slave/recipe_m... > File scripts/slave/recipe_modules/chromium_tests/steps.py (right): > > https://codereview.chromium.org/2063323002/diff/180001/scripts/slave/recipe_m... > scripts/slave/recipe_modules/chromium_tests/steps.py:1866: class > FindAnnotatedTest(Test): > On 2016/06/28 at 10:37:19, jbudorick (EMEA til June 30) wrote: > > Hm, I should've been more clear. This isn't what I meant, either. I'm instead > wondering about the feasibility of implementing this as a ScriptTest that gets > specified in src/testing/buildbot/chromium.fyi.json. > > After talking with Yoland it seems like making it a script test isnt the best > idea. This needs to upload some things to Google Storage and clean up temp files > after the script runs. Moving these things into a script would duplicate code > (we already have recipe APIs to do these things) and just make it more complex. > > https://codereview.chromium.org/2063323002/diff/180001/scripts/slave/recipe_m... > scripts/slave/recipe_modules/chromium_tests/steps.py:1867: test_apks = { > eh, _TEST_APKS probably. or _DEFAULT_TEST_APKS > > https://codereview.chromium.org/2063323002/diff/180001/scripts/slave/recipe_m... > scripts/slave/recipe_modules/chromium_tests/steps.py:1891: '--test-apks', ' > '.join(FindAnnotatedTest.test_apks.values()), > How does your script find the test apks? Should you be passing the fullpaths to > the apks to this script?
yolandyan@chromium.org changed reviewers: + thakis@chromium.org
thakis@chromium.org changed reviewers: + phajdan.jr@chromium.org - thakis@chromium.org
when adding people to a review, please say what you want them to do. here's how the email i got for this looked in my inbox: http://i.imgur.com/Xj2Bxne.png reading through comments, it looks like you're looking for a chomium_tests owner. according to scripts/slave/recipe_modules/chromium_tests/OWNERS i only own chromium_fyi.py. +phajdan: can you look at the chromium_tests bits here, please?
my bad! +phajdan.jr would you mind taking a look at both `scripts/slave/recipe_modules/chromium_tests/chromium_fyi.py` and `scripts/slave/recipe_modules/chromium_tests/steps.py` on whether you are okay with this approach vs adding a script in src/testing/scripts to define this step? https://codereview.chromium.org/2063323002/diff2/1:180001/scripts/slave/recip... https://codereview.chromium.org/2063323002/diff2/1:180001/scripts/slave/recip...
LGTM w/comment A script used to be preferred, but now we're close to moving all of this to chromium/src, and so it's actually easier in steps.py. Thanks for checking. :) https://codereview.chromium.org/2063323002/diff/180001/scripts/slave/recipe_m... File scripts/slave/recipe_modules/chromium_tests/steps.py (right): https://codereview.chromium.org/2063323002/diff/180001/scripts/slave/recipe_m... scripts/slave/recipe_modules/chromium_tests/steps.py:1901: api.file.rmtree('Delete temp out directory', temp_output_dir) Could you use tempfile.temp_dir context manager from the recipe engine?
https://codereview.chromium.org/2063323002/diff/180001/scripts/slave/recipe_m... File scripts/slave/recipe_modules/chromium_tests/steps.py (right): https://codereview.chromium.org/2063323002/diff/180001/scripts/slave/recipe_m... scripts/slave/recipe_modules/chromium_tests/steps.py:1867: test_apks = { On 2016/06/28 at 22:41:31, mikecase wrote: > eh, _TEST_APKS probably. or _DEFAULT_TEST_APKS Done
https://codereview.chromium.org/2063323002/diff/180001/scripts/slave/recipe_m... File scripts/slave/recipe_modules/chromium_tests/steps.py (right): https://codereview.chromium.org/2063323002/diff/180001/scripts/slave/recipe_m... scripts/slave/recipe_modules/chromium_tests/steps.py:1901: api.file.rmtree('Delete temp out directory', temp_output_dir) On 2016/07/04 at 10:42:42, Paweł Hajdan Jr. wrote: > Could you use tempfile.temp_dir context manager from the recipe engine? Done
The CQ bit was checked by yolandyan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from phajdan.jr@chromium.org, mikecase@chromium.org Link to the patchset: https://codereview.chromium.org/2063323002/#ps200001 (title: "Use context manager to temp dir")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Build Presubmit on master.tryserver.infra (JOB_FAILED, https://build.chromium.org/p/tryserver.infra/builders/Build%20Presubmit/build...)
The CQ bit was checked by yolandyan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from phajdan.jr@chromium.org, mikecase@chromium.org Link to the patchset: https://codereview.chromium.org/2063323002/#ps280001 (title: "Pass branch info to Nightly scheduler")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Create recipe to run find_annotated_tests.py periodically BUG=601909 ========== to ========== Create recipe to run find_annotated_tests.py periodically BUG=601909 Committed: https://chromium.googlesource.com/chromium/tools/build/+/b8aaec337558e95483a4... ==========
Message was sent while issue was closed.
Committed patchset #15 (id:280001) as https://chromium.googlesource.com/chromium/tools/build/+/b8aaec337558e95483a4... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
