New issue
Advanced search Search tips

Issue 883150 link

Starred by 1 user
Status: Assigned
Owner:
martiniss@chromium.org
Cc:
bpastene@chromium.org
dpranke@chromium.org
jbudorick@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug



Sign in to add a comment

Refactor class structure in chromium_tests/steps.py

Project Member Reported by martiniss@chromium.org, Sep 12 
Currently the class structure is suboptimal. 

Generally, there isn't a good reason (at least that I'm aware of) that there needs to be a distinction between isolated script tests, gtests, and other tests. There could be a few, but AFAIK we should be able to merge them all together.

The 'Test' base class also has a bunch of methods, most of which aren't applicable for classes which aren't isolated script tests or gtests. There are several pylint warnings about abstract methods not being overrided, which seems to point to this. 

This is more of a general tracking bug to improve the class structure in steps.py.
Project Member

Comment 1 by bugdroid1@chromium.org, Sep 12 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/tools/build/+/f5310466ab2c1fc49f1bbe7a63794fb30a8e1774

commit f5310466ab2c1fc49f1bbe7a63794fb30a8e1774
Author: Stephen Martinis <martiniss@chromium.org>
Date: Wed Sep 12 18:55:28 2018

steps.py: Fix pylint errors

Disables the abstract-method warning. That will be fixed in a subsequent CL
which restructures the class structure in the file.

Bug: 883150
Change-Id: Ifb349a62db19bf066c73e48eff30b68f87b02d08
Reviewed-on: https://chromium-review.googlesource.com/1220052
Commit-Queue: Stephen Martinis <martiniss@chromium.org>
Reviewed-by: Ben Pastene <bpastene@chromium.org>
Auto-Submit: Stephen Martinis <martiniss@chromium.org>

[modify] https://crrev.com/f5310466ab2c1fc49f1bbe7a63794fb30a8e1774/scripts/slave/recipe_modules/chromium_tests/steps.py

Comment 2 by martiniss@chromium.org, Sep 13

Owner: martiniss@chromium.org
Status: Assigned (was: Available)
Project Member

Comment 3 by bugdroid1@chromium.org, Sep 13 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/tools/build/+/9c78c5d67115fc4bed1187538569fe7dac7a7638

commit 9c78c5d67115fc4bed1187538569fe7dac7a7638
Author: Stephen Martinis <martiniss@chromium.org>
Date: Thu Sep 13 22:09:45 2018

Remove ArchiveBuildStep

This functionality is already present using the 'archive_build'
bot_config property. It doesn't need to be a test step.

Bug: 883150
Change-Id: Ia0805816478295866bc9e51663f3fdbef142aabb
Reviewed-on: https://chromium-review.googlesource.com/1225413
Commit-Queue: Stephen Martinis <martiniss@chromium.org>
Reviewed-by: John Budorick <jbudorick@chromium.org>
Reviewed-by: Ben Pastene <bpastene@chromium.org>

[modify] https://crrev.com/9c78c5d67115fc4bed1187538569fe7dac7a7638/scripts/slave/recipe_modules/chromium_tests/client_v8_fyi.py
[delete] https://crrev.com/120bedafbc036e6b52511c70b76ee3c74a9d7d55/scripts/slave/recipe_modules/chromium_tests/tests/steps/archive_build_step.py
[modify] https://crrev.com/9c78c5d67115fc4bed1187538569fe7dac7a7638/scripts/slave/README.recipes.md
[modify] https://crrev.com/9c78c5d67115fc4bed1187538569fe7dac7a7638/scripts/slave/recipe_modules/chromium_tests/chromium_chromiumos.py
[modify] https://crrev.com/9c78c5d67115fc4bed1187538569fe7dac7a7638/scripts/slave/recipe_modules/chromium/api.py
[modify] https://crrev.com/9c78c5d67115fc4bed1187538569fe7dac7a7638/scripts/slave/recipe_modules/chromium_tests/steps.py
[modify] https://crrev.com/9c78c5d67115fc4bed1187538569fe7dac7a7638/scripts/slave/recipe_modules/chromium/tests/archive_build.py
[add] https://crrev.com/9c78c5d67115fc4bed1187538569fe7dac7a7638/scripts/slave/recipe_modules/chromium/tests/archive_build.expected/experimental.json
Project Member

Comment 4 by bugdroid1@chromium.org, Sep 14 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/tools/build/+/9135fe60c879a8046ad700641b413859e2be4d4f

commit 9135fe60c879a8046ad700641b413859e2be4d4f
Author: Stephen Martinis <martiniss@chromium.org>
Date: Fri Sep 14 22:57:08 2018

Move has_valid_results() and failures() to base Test class

There's no real reason these methods need to be defined in each subclass.
Implement them in the base class.

Bug: 883150
Change-Id: Id37c28222992df66f544c63ff2072533aa55dd2c
Reviewed-on: https://chromium-review.googlesource.com/1225418
Commit-Queue: Stephen Martinis <martiniss@chromium.org>
Reviewed-by: Garrett Beaty <gbeaty@chromium.org>

[modify] https://crrev.com/9135fe60c879a8046ad700641b413859e2be4d4f/scripts/slave/recipes/webrtc/standalone.expected/client_webrtc_perf_mac_10_11.json
[modify] https://crrev.com/9135fe60c879a8046ad700641b413859e2be4d4f/scripts/slave/recipe_modules/chromium_tests/tests/steps/swarming_isolated_script_test.expected/chartjson_invalid.json
[modify] https://crrev.com/9135fe60c879a8046ad700641b413859e2be4d4f/scripts/slave/recipes/chromium.expected/dynamic_swarmed_sharded_invalid_json_isolated_script_test.json
[modify] https://crrev.com/9135fe60c879a8046ad700641b413859e2be4d4f/scripts/slave/recipes/chromium.expected/dynamic_swarmed_sharded_corrupt_json_isolated_script_test.json
[modify] https://crrev.com/9135fe60c879a8046ad700641b413859e2be4d4f/scripts/slave/recipe_modules/chromium_tests/tests/steps/script_test.py
[modify] https://crrev.com/9135fe60c879a8046ad700641b413859e2be4d4f/scripts/slave/recipes/webrtc/standalone.expected/client_webrtc_perf_linux_trusty.json
[modify] https://crrev.com/9135fe60c879a8046ad700641b413859e2be4d4f/scripts/slave/recipe_modules/chromium_tests/steps.py
[add] https://crrev.com/9135fe60c879a8046ad700641b413859e2be4d4f/scripts/slave/recipe_modules/chromium_tests/tests/steps/script_test.expected/invalid_results.json
[modify] https://crrev.com/9135fe60c879a8046ad700641b413859e2be4d4f/scripts/slave/recipes/webrtc/standalone.expected/client_webrtc_perf_win7.json
Project Member

Comment 5 by bugdroid1@chromium.org, Sep 27 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/tools/build/+/ed2d9b438b04571d3fd252b0229e5051ffd85af8

commit ed2d9b438b04571d3fd252b0229e5051ffd85af8
Author: Stephen Martinis <martiniss@chromium.org>
Date: Thu Sep 27 17:54:25 2018

chromium_tests: Fix pylint errors in api.py

Bug: 883150
Change-Id: Ie4b0cd5e5c0d6a7e8527bd1a58d879895d9e5b2e
Reviewed-on: https://chromium-review.googlesource.com/1247342
Reviewed-by: Garrett Beaty <gbeaty@chromium.org>
Commit-Queue: Stephen Martinis <martiniss@chromium.org>
Auto-Submit: Stephen Martinis <martiniss@chromium.org>

[modify] https://crrev.com/ed2d9b438b04571d3fd252b0229e5051ffd85af8/scripts/slave/recipe_modules/chromium_tests/bot_config_and_test_db.py
[modify] https://crrev.com/ed2d9b438b04571d3fd252b0229e5051ffd85af8/scripts/slave/recipe_modules/chromium_tests/api.py
Project Member

Comment 6 by bugdroid1@chromium.org, Dec 4 

The following revision refers to this bug:
  https://chrome-internal.googlesource.com/chrome/tools/release/scripts/+/938689b55f52ef1b6f893b61c9e2830e44391ffb

commit 938689b55f52ef1b6f893b61c9e2830e44391ffb
Author: Stephen Martinis <martiniss@google.com>
Date: Tue Dec 04 21:31:35 2018
Project Member

Comment 8 by bugdroid1@chromium.org, Dec 5 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/tools/build/+/8c6980875182ce699b07eb5500016bbce907970a

commit 8c6980875182ce699b07eb5500016bbce907970a
Author: Stephen Martinis <martiniss@chromium.org>
Date: Wed Dec 05 21:45:54 2018

Move chromium_tests generators to a separate file

Just moving code and updating references. Should affect nothing.

Bug: 883150
Change-Id: I84168438422dfbc35ee020bcaf039dcddeaf0b45
Reviewed-on: https://chromium-review.googlesource.com/c/1360052
Commit-Queue: Stephen Martinis <martiniss@chromium.org>
Reviewed-by: John Budorick <jbudorick@chromium.org>

[modify] https://crrev.com/8c6980875182ce699b07eb5500016bbce907970a/scripts/slave/recipe_modules/chromium_tests/tests/steps/generate_instrumentation_test.py
[modify] https://crrev.com/8c6980875182ce699b07eb5500016bbce907970a/scripts/slave/recipe_modules/chromium_tests/tests/steps/generate_junit_test.py
[modify] https://crrev.com/8c6980875182ce699b07eb5500016bbce907970a/scripts/slave/recipe_modules/chromium_tests/tests/steps/generate_isolated_script.py
[modify] https://crrev.com/8c6980875182ce699b07eb5500016bbce907970a/scripts/slave/recipe_modules/chromium_tests/tests/steps/generate_gtest.py
[modify] https://crrev.com/8c6980875182ce699b07eb5500016bbce907970a/scripts/slave/recipe_modules/chromium_tests/bot_config_and_test_db.py
[modify] https://crrev.com/8c6980875182ce699b07eb5500016bbce907970a/scripts/slave/README.recipes.md
[modify] https://crrev.com/8c6980875182ce699b07eb5500016bbce907970a/scripts/slave/recipe_modules/chromium_tests/tests/steps/generate_cts_test.py
[add] https://crrev.com/8c6980875182ce699b07eb5500016bbce907970a/scripts/slave/recipe_modules/chromium_tests/generators.py
[modify] https://crrev.com/8c6980875182ce699b07eb5500016bbce907970a/scripts/slave/recipe_modules/chromium_tests/steps.py
[modify] https://crrev.com/8c6980875182ce699b07eb5500016bbce907970a/scripts/slave/recipe_modules/chromium_tests/api.py
[modify] https://crrev.com/8c6980875182ce699b07eb5500016bbce907970a/scripts/slave/recipe_modules/chromium_tests/tests/steps/get_args_for_test.py
[modify] https://crrev.com/8c6980875182ce699b07eb5500016bbce907970a/scripts/slave/recipe_modules/chromium_tests/tests/steps/generate_fuchsia_test.py
[modify] https://crrev.com/8c6980875182ce699b07eb5500016bbce907970a/scripts/slave/recipe_modules/chromium_tests/tests/steps/generate_script.py
Project Member

Comment 9 by bugdroid1@chromium.org, Dec 7 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/tools/build/+/94c169887ebcffbc3400258cbbec050f65a131d4

commit 94c169887ebcffbc3400258cbbec050f65a131d4
Author: Stephen Martinis <martiniss@chromium.org>
Date: Fri Dec 07 01:01:15 2018

Refactor swarming create_task

Adds a common create task which is used by both
SwarmingIsolatedScriptTest and SwarmingGTestTest. There was a lot of
code duplication previously.

Bug: 883150
Change-Id: I8a3b9e3fd8de56c752a3f23badcbd97ccf252e9c
Reviewed-on: https://chromium-review.googlesource.com/c/1361883
Reviewed-by: Garrett Beaty <gbeaty@chromium.org>
Commit-Queue: Stephen Martinis <martiniss@chromium.org>

[modify] https://crrev.com/94c169887ebcffbc3400258cbbec050f65a131d4/scripts/slave/recipe_modules/chromium_tests/tests/steps/swarming_isolated_script_test.expected/without_patch_filter.json
[modify] https://crrev.com/94c169887ebcffbc3400258cbbec050f65a131d4/scripts/slave/recipe_modules/chromium_tests/steps.py
[modify] https://crrev.com/94c169887ebcffbc3400258cbbec050f65a131d4/scripts/slave/recipes/chromium_trybot.expected/dynamic_isolated_script_test_on_trybot_failing.json
[modify] https://crrev.com/94c169887ebcffbc3400258cbbec050f65a131d4/scripts/slave/recipes/chromium_trybot.expected/swarmed_webkit_tests_interrupted.json
[modify] https://crrev.com/94c169887ebcffbc3400258cbbec050f65a131d4/scripts/slave/recipes/chromium_trybot.expected/swarmed_webkit_tests_unexpected_error.json
[modify] https://crrev.com/94c169887ebcffbc3400258cbbec050f65a131d4/scripts/slave/recipes/chromium_trybot.expected/swarmed_layout_tests_too_many_failures_for_retcode.json
►

Sign in to add a comment