New issue
Advanced search Search tips

Issue 878777 link

Starred by 2 users
Status: Assigned
Owner:
jbudorick@chromium.org
Cc:
perezju@chromium.org
ntfschr@chromium.org
jbudorick@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug

Blocked on:
issue 777865


Sign in to add a comment

presubmit or a wrapper around Mock/MagicMock against common mistakes with assert_called_once() and assert_not_called()

Project Member Reported by pasko@chromium.org, Aug 29 
I'm puzzled by this repro:

1. system_app_test.py has a check to verify that method EnableRoot() was called on DeviceUtils object: 

https://codesearch.chromium.org/chromium/src/third_party/catapult/devil/devil/android/tools/system_app_test.py?q=third_party/catapult/devil/devil/android/tools/system_app_test.py&sq=package:chromium&g=0&l=50

2. the test executes mock_device.EnableRoot.assert_called_once() unconditionally, and passes when executed:

shell> cd third_party/catapult && ./devil/bin/run_py_tests
590 tests passed, 0 skipped, 0 failures.

3. if we add 'assert False' just after that line, the test fails:

shell> 
  Traceback (most recent call last):
    File "/path/to/src/third_party/catapult/devil/devil/android/tools/system_app_test.py", line 51, in testDoubleEnableModification
      assert False
  AssertionError
589 tests passed, 0 skipped, 1 failure.

So far so good..

4. if we replace the line with 'mock_device.EnableRoot.assert_not_called()' the tests still pass!

Is this an incorrect assert or the mock asserts are somehow disabled altogether?

Comment 1 Deleted

Comment 2 by pasko@chromium.org, Aug 29 

also, is there a way to run a single test? Ability for pdb.set_trace() to drop into interactive console would be extra appreciated :)

Comment 3 by perezju@chromium.org, Aug 29 

I think we're hitting:
https://engineeringblog.yelp.com/2015/02/assert_called_once-threat-or-menace.html

"assert_called_once" does not exist, it should be "assert_called_once_with".

Comment 4 by nednguyen@chromium.org, Aug 29 

Looks like devil test run script doesn't hook the test filtering from typ yet. For pdb, you can run:

./devil/bin/run_py_tests -j 1 --passthrough

Comment 5 by perezju@chromium.org, Aug 29 

I seem to remember it does some sort of filtering, but you need to pass the fully qualified name of the test you want to run (e.g. devl.android.tools.system_app_test...)

Comment 6 by pasko@chromium.org, Aug 29 

oh, indeed your copy of mock does not have assert_called_once, so replacing with mock_device.EnableRoot.assert_whatever_comes_to_mind() also passes the test :)

for pdb: great thanks!!

Comment 7 by pasko@chromium.org, Aug 29 

indeed referring a test by name works:
shell> ./devil/bin/run_py_tests -j 1 --passthrough devil.android.tools.system_app_test.SystemAppTest.testDoubleEnableModification
1 test passed, 0 skipped, 0 failures.

thank you!!

Comment 8 by pasko@chromium.org, Aug 29

Cc: -jbudorick@chromium.org
Owner: jbudorick@chromium.org
-> John

feel free to close as WAI, or to use it as tracking a presubmit check against assert_called_once() and assert_not_called()

Comment 9 by pasko@chromium.org, Aug 29

Status: Assigned (was: Untriaged)
Project Member

Comment 10 by bugdroid1@chromium.org, Aug 29 

The following revision refers to this bug:
  https://chromium.googlesource.com/catapult/+/7dfd811ec1065d48a7b3a78096d99656dd4401ea

commit 7dfd811ec1065d48a7b3a78096d99656dd4401ea
Author: Egor Pasko <pasko@chromium.org>
Date: Wed Aug 29 16:57:41 2018

devil: Replace mock asserts with the working ones

The current Python Mock library we are using does not support
'assert_called_once' and 'assert_not_called' checks on the mocks.
Replace them with less pretty alternatives that seem to work.

There are other uses in catapult, this change only makes replacements in
devil/.

Bug: chromium:878777
Change-Id: Icc34e0069c5a229cd9e363f3ac8945494169e0d0
Reviewed-on: https://chromium-review.googlesource.com/1193825
Reviewed-by: Juan Antonio Navarro Pérez <perezju@chromium.org>
Commit-Queue: Egor Pasko <pasko@chromium.org>

[modify] https://crrev.com/7dfd811ec1065d48a7b3a78096d99656dd4401ea/devil/devil/utils/lazy/weak_constant_test.py
[modify] https://crrev.com/7dfd811ec1065d48a7b3a78096d99656dd4401ea/devil/devil/android/tools/system_app_test.py
Project Member

Comment 11 by bugdroid1@chromium.org, Aug 29 

The following revision refers to this bug:
  https://chromium.googlesource.com/catapult/+/c79e532e2058b8c8ec1dd1e6086fead046eed097

commit c79e532e2058b8c8ec1dd1e6086fead046eed097
Author: Egor Pasko <pasko@chromium.org>
Date: Wed Aug 29 17:58:01 2018

dashboard: replace assert_called_once()

The third_party/mock/mock.py does not support assert_called_once(), it
is equivalent to a noop. Changing it to assert_called_once_with().

The bug has more details:

Bug: chromium:878777
Change-Id: I8bdd6ed31dec585e79de8f83e98637f88ed49e40
Reviewed-on: https://chromium-review.googlesource.com/1193828
Reviewed-by: Simon Hatch <simonhatch@chromium.org>
Commit-Queue: Egor Pasko <pasko@chromium.org>

[modify] https://crrev.com/c79e532e2058b8c8ec1dd1e6086fead046eed097/dashboard/dashboard/add_histograms_test.py

Comment 12 by jbudorick@chromium.org, Aug 29 

I think we definitely either want a presubmit or a wrapper around Mock/MagicMock for this.
Project Member

Comment 13 by bugdroid1@chromium.org, Aug 29 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/51390a3d63114872fe1b0adb9c454bd527d5747e

commit 51390a3d63114872fe1b0adb9c454bd527d5747e
Author: catapult-chromium-autoroll <catapult-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Date: Wed Aug 29 19:49:50 2018

Roll src/third_party/catapult 6443c8dade1c..7dfd811ec106 (1 commits)

https://chromium.googlesource.com/catapult.git/+log/6443c8dade1c..7dfd811ec106


git log 6443c8dade1c..7dfd811ec106 --date=short --no-merges --format='%ad %ae %s'
2018-08-29 pasko@chromium.org devil: Replace mock asserts with the working ones


Created with:
  gclient setdep -r src/third_party/catapult@7dfd811ec106

The AutoRoll server is located here: https://autoroll.skia.org/r/catapult-autoroll

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

If the roll is causing failures, please contact the current sheriff, who should
be CC'd on the roll, and stop the roller if necessary.

CQ_INCLUDE_TRYBOTS=luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel

BUG=chromium:878777
TBR=sullivan@chromium.org

Change-Id: I2e1436b377593c4c838a80acb6b1eeb5e5e1c66c
Reviewed-on: https://chromium-review.googlesource.com/1195054
Reviewed-by: catapult-chromium-autoroll <catapult-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Commit-Queue: catapult-chromium-autoroll <catapult-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Cr-Commit-Position: refs/heads/master@{#587255}
[modify] https://crrev.com/51390a3d63114872fe1b0adb9c454bd527d5747e/DEPS

Comment 14 by pasko@chromium.org, Aug 29

Summary: presubmit or a wrapper around Mock/MagicMock against common mistakes with assert_called_once() and assert_not_called() (was: devil: pymock assert_called_once() and assert_not_called() not working properly)
Project Member

Comment 15 by bugdroid1@chromium.org, Aug 29 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3bdd397e4385c543178d8e7b4abd0eb7bb03b987

commit 3bdd397e4385c543178d8e7b4abd0eb7bb03b987
Author: catapult-chromium-autoroll <catapult-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Date: Wed Aug 29 22:05:32 2018

Roll src/third_party/catapult 7dfd811ec106..8972cd776e13 (6 commits)

https://chromium.googlesource.com/catapult.git/+log/7dfd811ec106..8972cd776e13


git log 7dfd811ec106..8972cd776e13 --date=short --no-merges --format='%ad %ae %s'
2018-08-29 dtu@chromium.org [pinpoint] Add temporary mapping from Buildbot to LUCI builders.
2018-08-29 simonhatch@chromium.org Pinpoint - Escape names when creating test path
2018-08-29 simonhatch@chromium.org Reland "Pinpoint - Include doc link in bug comment."
2018-08-29 pasko@chromium.org dashboard: replace assert_called_once()
2018-08-29 zmo@chromium.org Remove node_runner from telemetry isolate
2018-08-29 dtu@chromium.org [pinpoint] d3-based heatmap and histogram.


Created with:
  gclient setdep -r src/third_party/catapult@8972cd776e13

The AutoRoll server is located here: https://autoroll.skia.org/r/catapult-autoroll

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

If the roll is causing failures, please contact the current sheriff, who should
be CC'd on the roll, and stop the roller if necessary.

CQ_INCLUDE_TRYBOTS=luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel

BUG=chromium:790075, chromium:863861 ,chromium:878777, chromium:872521 
TBR=sullivan@chromium.org

Change-Id: I17642df1db4953b723db72b7b52caaf3a97d8538
Reviewed-on: https://chromium-review.googlesource.com/1195862
Reviewed-by: catapult-chromium-autoroll <catapult-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Commit-Queue: catapult-chromium-autoroll <catapult-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Cr-Commit-Position: refs/heads/master@{#587303}
[modify] https://crrev.com/3bdd397e4385c543178d8e7b4abd0eb7bb03b987/DEPS
Project Member

Comment 16 by bugdroid1@chromium.org, Sep 21 

The following revision refers to this bug:
  https://chromium.googlesource.com/catapult/+/23c67a511280f6dbc2a2bea849dd7eed5104809e

commit 23c67a511280f6dbc2a2bea849dd7eed5104809e
Author: John Budorick <jbudorick@chromium.org>
Date: Fri Sep 21 14:19:21 2018

Use vpython and remove vendored pymock.

Bug: chromium:878777
Bug: chromium:777865
Change-Id: If0a536516a2380423e3d7eef218c46123c78fedd
Reviewed-on: https://chromium-review.googlesource.com/1199813
Commit-Queue: John Budorick <jbudorick@chromium.org>
Reviewed-by: Ned Nguyen <nednguyen@google.com>
Reviewed-by: Ben Pastene <bpastene@chromium.org>
Reviewed-by: Juan Antonio Navarro Pérez <perezju@chromium.org>

[modify] https://crrev.com/23c67a511280f6dbc2a2bea849dd7eed5104809e/devil/bin/run_py_devicetests
[modify] https://crrev.com/23c67a511280f6dbc2a2bea849dd7eed5104809e/common/bin/run_tests
[modify] https://crrev.com/23c67a511280f6dbc2a2bea849dd7eed5104809e/tracing/bin/run_vinn_tests
[modify] https://crrev.com/23c67a511280f6dbc2a2bea849dd7eed5104809e/dashboard/bin/run_dev_server_tests
[modify] https://crrev.com/23c67a511280f6dbc2a2bea849dd7eed5104809e/devil/devil/android/tools/system_app_test.py
[modify] https://crrev.com/23c67a511280f6dbc2a2bea849dd7eed5104809e/devil/devil/utils/lazy/weak_constant_test.py
[modify] https://crrev.com/23c67a511280f6dbc2a2bea849dd7eed5104809e/devil/devil/utils/cmd_helper_test.py
[modify] https://crrev.com/23c67a511280f6dbc2a2bea849dd7eed5104809e/systrace/bin/run_tests
[modify] https://crrev.com/23c67a511280f6dbc2a2bea849dd7eed5104809e/telemetry/bin/list_telemetry_unittests
[modify] https://crrev.com/23c67a511280f6dbc2a2bea849dd7eed5104809e/experimental/bisect_lib/fetch_revision_info_test.py
[modify] https://crrev.com/23c67a511280f6dbc2a2bea849dd7eed5104809e/devil/devil/android/sdk/shared_prefs_test.py
[modify] https://crrev.com/23c67a511280f6dbc2a2bea849dd7eed5104809e/telemetry/telemetry/internal/util/binary_manager_unittest.py
[modify] https://crrev.com/23c67a511280f6dbc2a2bea849dd7eed5104809e/common/py_utils/py_utils/__init__.py
[modify] https://crrev.com/23c67a511280f6dbc2a2bea849dd7eed5104809e/third_party/vinn/vinn/vinn_unittest.py
[modify] https://crrev.com/23c67a511280f6dbc2a2bea849dd7eed5104809e/catapult_build/PRESUBMIT.py
[modify] https://crrev.com/23c67a511280f6dbc2a2bea849dd7eed5104809e/devil/devil/android/tools/device_monitor_test.py
[modify] https://crrev.com/23c67a511280f6dbc2a2bea849dd7eed5104809e/devil/devil/android/apk_helper_test.py
[modify] https://crrev.com/23c67a511280f6dbc2a2bea849dd7eed5104809e/telemetry/bin/run_snap_it_unittest
[modify] https://crrev.com/23c67a511280f6dbc2a2bea849dd7eed5104809e/telemetry/PRESUBMIT.py
[modify] https://crrev.com/23c67a511280f6dbc2a2bea849dd7eed5104809e/devil/devil/android/sdk/adb_wrapper_test.py
[modify] https://crrev.com/23c67a511280f6dbc2a2bea849dd7eed5104809e/devil/bin/run_py_tests
[modify] https://crrev.com/23c67a511280f6dbc2a2bea849dd7eed5104809e/devil/devil/utils/mock_calls.py
[modify] https://crrev.com/23c67a511280f6dbc2a2bea849dd7eed5104809e/devil/devil/android/fastboot_utils_test.py
[modify] https://crrev.com/23c67a511280f6dbc2a2bea849dd7eed5104809e/telemetry/bin/run_browser_tests
[modify] https://crrev.com/23c67a511280f6dbc2a2bea849dd7eed5104809e/third_party/vinn/bin/run_tests
[modify] https://crrev.com/23c67a511280f6dbc2a2bea849dd7eed5104809e/devil/devil/android/battery_utils_test.py
[modify] https://crrev.com/23c67a511280f6dbc2a2bea849dd7eed5104809e/dependency_manager/PRESUBMIT.py
[modify] https://crrev.com/23c67a511280f6dbc2a2bea849dd7eed5104809e/catapult_build/bin/run_py_tests
[modify] https://crrev.com/23c67a511280f6dbc2a2bea849dd7eed5104809e/netlog_viewer/bin/run_dev_server_tests
[modify] https://crrev.com/23c67a511280f6dbc2a2bea849dd7eed5104809e/devil/devil/android/logcat_monitor_test.py
[delete] https://crrev.com/0a7a46e7b9764220b64f88466cba845589c39e10/third_party/mock/README.chromium
[modify] https://crrev.com/23c67a511280f6dbc2a2bea849dd7eed5104809e/dashboard/PRESUBMIT.py
[modify] https://crrev.com/23c67a511280f6dbc2a2bea849dd7eed5104809e/common/eslint/bin/run_tests
[modify] https://crrev.com/23c67a511280f6dbc2a2bea849dd7eed5104809e/dependency_manager/bin/run_tests
[modify] https://crrev.com/23c67a511280f6dbc2a2bea849dd7eed5104809e/common/py_utils/PRESUBMIT.py
[modify] https://crrev.com/23c67a511280f6dbc2a2bea849dd7eed5104809e/devil/devil/android/tools/script_common_test.py
[modify] https://crrev.com/23c67a511280f6dbc2a2bea849dd7eed5104809e/telemetry/telemetry/internal/platform/linux_based_platform_backend_unittest.py
[modify] https://crrev.com/23c67a511280f6dbc2a2bea849dd7eed5104809e/devil/devil/utils/lsusb_test.py
[modify] https://crrev.com/23c67a511280f6dbc2a2bea849dd7eed5104809e/dashboard/bin/run_py_tests
[modify] https://crrev.com/23c67a511280f6dbc2a2bea849dd7eed5104809e/devil/devil/android/perf/perf_control_test.py
[modify] https://crrev.com/23c67a511280f6dbc2a2bea849dd7eed5104809e/BUILD.gn
[modify] https://crrev.com/23c67a511280f6dbc2a2bea849dd7eed5104809e/systrace/PRESUBMIT.py
[modify] https://crrev.com/23c67a511280f6dbc2a2bea849dd7eed5104809e/tracing/bin/run_symbolizer_tests
[modify] https://crrev.com/23c67a511280f6dbc2a2bea849dd7eed5104809e/telemetry/bin/run_tests
[modify] https://crrev.com/23c67a511280f6dbc2a2bea849dd7eed5104809e/devil/devil/android/md5sum_test.py
[modify] https://crrev.com/23c67a511280f6dbc2a2bea849dd7eed5104809e/catapult_build/fetch_telemetry_deps_and_run_tests
[modify] https://crrev.com/23c67a511280f6dbc2a2bea849dd7eed5104809e/devil/devil/devil_env.py
[modify] https://crrev.com/23c67a511280f6dbc2a2bea849dd7eed5104809e/.vpython
[modify] https://crrev.com/23c67a511280f6dbc2a2bea849dd7eed5104809e/devil/devil/utils/find_usb_devices_test.py
[modify] https://crrev.com/23c67a511280f6dbc2a2bea849dd7eed5104809e/telemetry/telemetry/__init__.py
[modify] https://crrev.com/23c67a511280f6dbc2a2bea849dd7eed5104809e/experimental/bisect_lib/fetch_intervening_revisions_test.py
[modify] https://crrev.com/23c67a511280f6dbc2a2bea849dd7eed5104809e/common/py_utils/py_utils/cloud_storage_unittest.py
[modify] https://crrev.com/23c67a511280f6dbc2a2bea849dd7eed5104809e/devil/devil/android/device_utils_test.py
[modify] https://crrev.com/23c67a511280f6dbc2a2bea849dd7eed5104809e/tracing/bin/run_py_tests
[modify] https://crrev.com/23c67a511280f6dbc2a2bea849dd7eed5104809e/tracing/PRESUBMIT.py
[modify] https://crrev.com/23c67a511280f6dbc2a2bea849dd7eed5104809e/tracing/bin/run_dev_server_tests
[modify] https://crrev.com/23c67a511280f6dbc2a2bea849dd7eed5104809e/common/py_vulcanize/bin/run_py_tests
[modify] https://crrev.com/23c67a511280f6dbc2a2bea849dd7eed5104809e/devil/devil/devil_env_test.py
[modify] https://crrev.com/23c67a511280f6dbc2a2bea849dd7eed5104809e/dependency_manager/dependency_manager/base_config_unittest.py
[modify] https://crrev.com/23c67a511280f6dbc2a2bea849dd7eed5104809e/dependency_manager/dependency_manager/__init__.py
[modify] https://crrev.com/23c67a511280f6dbc2a2bea849dd7eed5104809e/devil/devil/android/app_ui_test.py
[modify] https://crrev.com/23c67a511280f6dbc2a2bea849dd7eed5104809e/tracing/bin/run_tests
[modify] https://crrev.com/23c67a511280f6dbc2a2bea849dd7eed5104809e/devil/devil/utils/mock_calls_test.py
[modify] https://crrev.com/23c67a511280f6dbc2a2bea849dd7eed5104809e/experimental/PRESUBMIT.py
[delete] https://crrev.com/0a7a46e7b9764220b64f88466cba845589c39e10/third_party/mock/mock.py

Comment 17 by jbudorick@chromium.org, Sep 21

Status: Fixed (was: Assigned)
Solved a bit differently than I initially planned, by upgrading to a newer version of mock delivered by vpython. The new version not only supports the newer functions we'd previously been trying to use, it also detects bad uses of non-existent things like assert_call_once_with (e.g. https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/23c67a511280f6dbc2a2bea849dd7eed5104809e%5E%21/#F53)

Comment 18 by nednguyen@chromium.org, Sep 21 

Awesome, thanks for doing this John!
Project Member

Comment 19 by bugdroid1@chromium.org, Sep 21 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a5725a11968896687d4e69d544f301ae4c01c034

commit a5725a11968896687d4e69d544f301ae4c01c034
Author: John Budorick <jbudorick@chromium.org>
Date: Fri Sep 21 20:24:01 2018

Add mock to //.vpython.

Bug: 878777
Change-Id: I7d6727fd36523f37db5f670a46e0957b93cb575d
Reviewed-on: https://chromium-review.googlesource.com/1239166
Commit-Queue: John Budorick <jbudorick@chromium.org>
Commit-Queue: Ben Pastene <bpastene@chromium.org>
Reviewed-by: Ben Pastene <bpastene@chromium.org>
Cr-Commit-Position: refs/heads/master@{#593320}
[modify] https://crrev.com/a5725a11968896687d4e69d544f301ae4c01c034/.vpython
Project Member

Comment 20 by bugdroid1@chromium.org, Sep 21 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/670f4d35ef9aaa5c6a8f194d71e7e4bf06539d38

commit 670f4d35ef9aaa5c6a8f194d71e7e4bf06539d38
Author: chromium-autoroll <chromium-autoroll@skia-public.iam.gserviceaccount.com>
Date: Fri Sep 21 22:18:05 2018

Roll src/third_party/catapult 15abb5c68f71..32261ee689fe (4 commits)

https://chromium.googlesource.com/catapult.git/+log/15abb5c68f71..32261ee689fe


git log 15abb5c68f71..32261ee689fe --date=short --no-merges --format='%ad %ae %s'
2018-09-21 sadrul@chromium.org dashboard: Include metric name for rendering alerts.
2018-09-21 benjhayden@chromium.org Fix bbc test case in system_health_report
2018-09-21 jbudorick@chromium.org Use vpython and remove vendored pymock.
2018-09-21 anthonyalridge@google.com Introduces state management for results.vue.


Created with:
  gclient setdep -r src/third_party/catapult@32261ee689fe

The AutoRoll server is located here: https://autoroll.skia.org/r/catapult-autoroll

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

If the roll is causing failures, please contact the current sheriff, who should
be CC'd on the roll, and stop the roller if necessary.

CQ_INCLUDE_TRYBOTS=luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel

BUG=chromium:870324,chromium:878777,chromium:777865,chromium:866423
TBR=sullivan@chromium.org

Change-Id: I42b47c5cb63f2cac87c9c82cbbdaf9b7eea42e9e
Reviewed-on: https://chromium-review.googlesource.com/1239158
Reviewed-by: chromium-autoroll <chromium-autoroll@skia-public.iam.gserviceaccount.com>
Commit-Queue: chromium-autoroll <chromium-autoroll@skia-public.iam.gserviceaccount.com>
Cr-Commit-Position: refs/heads/master@{#593355}
[modify] https://crrev.com/670f4d35ef9aaa5c6a8f194d71e7e4bf06539d38/DEPS
Project Member

Comment 22 by bugdroid1@chromium.org, Sep 24 

The following revision refers to this bug:
  https://chromium.googlesource.com/catapult/+/64f2ed4bb2f1a094bba38cb817ed912a118e442e

commit 64f2ed4bb2f1a094bba38cb817ed912a118e442e
Author: John Budorick <jbudorick@chromium.org>
Date: Mon Sep 24 18:23:40 2018

Revert "Use vpython and remove vendored pymock."

This reverts commit 23c67a511280f6dbc2a2bea849dd7eed5104809e.

Reason for revert: Causing issues for both pinpoint and cros pfq ( crbug.com/888574 )

Original change's description:
> Use vpython and remove vendored pymock.
>
> Bug: chromium:878777
> Bug: chromium:777865
> Change-Id: If0a536516a2380423e3d7eef218c46123c78fedd
> Reviewed-on: https://chromium-review.googlesource.com/1199813
> Commit-Queue: John Budorick <jbudorick@chromium.org>
> Reviewed-by: Ned Nguyen <nednguyen@google.com>
> Reviewed-by: Ben Pastene <bpastene@chromium.org>
> Reviewed-by: Juan Antonio Navarro Pérez <perezju@chromium.org>

TBR=perezju@chromium.org,nednguyen@google.com,bpastene@chromium.org,jbudorick@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: chromium:878777
Bug: chromium:777865
Change-Id: I60eb9ebdeb50e74f532daff619ecb66577afbcbb
Reviewed-on: https://chromium-review.googlesource.com/1240593
Commit-Queue: John Budorick <jbudorick@chromium.org>
Reviewed-by: John Budorick <jbudorick@chromium.org>

[modify] https://crrev.com/64f2ed4bb2f1a094bba38cb817ed912a118e442e/devil/bin/run_py_devicetests
[modify] https://crrev.com/64f2ed4bb2f1a094bba38cb817ed912a118e442e/common/bin/run_tests
[modify] https://crrev.com/64f2ed4bb2f1a094bba38cb817ed912a118e442e/tracing/bin/run_vinn_tests
[modify] https://crrev.com/64f2ed4bb2f1a094bba38cb817ed912a118e442e/dashboard/bin/run_dev_server_tests
[modify] https://crrev.com/64f2ed4bb2f1a094bba38cb817ed912a118e442e/devil/devil/android/tools/system_app_test.py
[modify] https://crrev.com/64f2ed4bb2f1a094bba38cb817ed912a118e442e/devil/devil/utils/lazy/weak_constant_test.py
[modify] https://crrev.com/64f2ed4bb2f1a094bba38cb817ed912a118e442e/devil/devil/utils/cmd_helper_test.py
[modify] https://crrev.com/64f2ed4bb2f1a094bba38cb817ed912a118e442e/systrace/bin/run_tests
[modify] https://crrev.com/64f2ed4bb2f1a094bba38cb817ed912a118e442e/telemetry/bin/list_telemetry_unittests
[modify] https://crrev.com/64f2ed4bb2f1a094bba38cb817ed912a118e442e/experimental/bisect_lib/fetch_revision_info_test.py
[modify] https://crrev.com/64f2ed4bb2f1a094bba38cb817ed912a118e442e/devil/devil/android/sdk/shared_prefs_test.py
[modify] https://crrev.com/64f2ed4bb2f1a094bba38cb817ed912a118e442e/telemetry/telemetry/internal/util/binary_manager_unittest.py
[modify] https://crrev.com/64f2ed4bb2f1a094bba38cb817ed912a118e442e/common/py_utils/py_utils/__init__.py
[modify] https://crrev.com/64f2ed4bb2f1a094bba38cb817ed912a118e442e/third_party/vinn/vinn/vinn_unittest.py
[modify] https://crrev.com/64f2ed4bb2f1a094bba38cb817ed912a118e442e/catapult_build/PRESUBMIT.py
[modify] https://crrev.com/64f2ed4bb2f1a094bba38cb817ed912a118e442e/devil/devil/android/tools/device_monitor_test.py
[modify] https://crrev.com/64f2ed4bb2f1a094bba38cb817ed912a118e442e/devil/devil/android/apk_helper_test.py
[modify] https://crrev.com/64f2ed4bb2f1a094bba38cb817ed912a118e442e/telemetry/bin/run_snap_it_unittest
[modify] https://crrev.com/64f2ed4bb2f1a094bba38cb817ed912a118e442e/telemetry/PRESUBMIT.py
[modify] https://crrev.com/64f2ed4bb2f1a094bba38cb817ed912a118e442e/devil/devil/android/sdk/adb_wrapper_test.py
[modify] https://crrev.com/64f2ed4bb2f1a094bba38cb817ed912a118e442e/devil/bin/run_py_tests
[modify] https://crrev.com/64f2ed4bb2f1a094bba38cb817ed912a118e442e/devil/devil/utils/mock_calls.py
[modify] https://crrev.com/64f2ed4bb2f1a094bba38cb817ed912a118e442e/devil/devil/android/fastboot_utils_test.py
[modify] https://crrev.com/64f2ed4bb2f1a094bba38cb817ed912a118e442e/telemetry/bin/run_browser_tests
[modify] https://crrev.com/64f2ed4bb2f1a094bba38cb817ed912a118e442e/third_party/vinn/bin/run_tests
[modify] https://crrev.com/64f2ed4bb2f1a094bba38cb817ed912a118e442e/devil/devil/android/battery_utils_test.py
[modify] https://crrev.com/64f2ed4bb2f1a094bba38cb817ed912a118e442e/dependency_manager/PRESUBMIT.py
[modify] https://crrev.com/64f2ed4bb2f1a094bba38cb817ed912a118e442e/catapult_build/bin/run_py_tests
[modify] https://crrev.com/64f2ed4bb2f1a094bba38cb817ed912a118e442e/netlog_viewer/bin/run_dev_server_tests
[modify] https://crrev.com/64f2ed4bb2f1a094bba38cb817ed912a118e442e/devil/devil/android/logcat_monitor_test.py
[add] https://crrev.com/64f2ed4bb2f1a094bba38cb817ed912a118e442e/third_party/mock/README.chromium
[modify] https://crrev.com/64f2ed4bb2f1a094bba38cb817ed912a118e442e/dashboard/PRESUBMIT.py
[modify] https://crrev.com/64f2ed4bb2f1a094bba38cb817ed912a118e442e/common/eslint/bin/run_tests
[modify] https://crrev.com/64f2ed4bb2f1a094bba38cb817ed912a118e442e/dependency_manager/bin/run_tests
[modify] https://crrev.com/64f2ed4bb2f1a094bba38cb817ed912a118e442e/common/py_utils/PRESUBMIT.py
[modify] https://crrev.com/64f2ed4bb2f1a094bba38cb817ed912a118e442e/devil/devil/android/tools/script_common_test.py
[modify] https://crrev.com/64f2ed4bb2f1a094bba38cb817ed912a118e442e/telemetry/telemetry/internal/platform/linux_based_platform_backend_unittest.py
[modify] https://crrev.com/64f2ed4bb2f1a094bba38cb817ed912a118e442e/devil/devil/utils/lsusb_test.py
[modify] https://crrev.com/64f2ed4bb2f1a094bba38cb817ed912a118e442e/dashboard/bin/run_py_tests
[modify] https://crrev.com/64f2ed4bb2f1a094bba38cb817ed912a118e442e/devil/devil/android/perf/perf_control_test.py
[modify] https://crrev.com/64f2ed4bb2f1a094bba38cb817ed912a118e442e/BUILD.gn
[modify] https://crrev.com/64f2ed4bb2f1a094bba38cb817ed912a118e442e/systrace/PRESUBMIT.py
[modify] https://crrev.com/64f2ed4bb2f1a094bba38cb817ed912a118e442e/tracing/bin/run_symbolizer_tests
[modify] https://crrev.com/64f2ed4bb2f1a094bba38cb817ed912a118e442e/telemetry/bin/run_tests
[modify] https://crrev.com/64f2ed4bb2f1a094bba38cb817ed912a118e442e/devil/devil/android/md5sum_test.py
[modify] https://crrev.com/64f2ed4bb2f1a094bba38cb817ed912a118e442e/catapult_build/fetch_telemetry_deps_and_run_tests
[modify] https://crrev.com/64f2ed4bb2f1a094bba38cb817ed912a118e442e/devil/devil/devil_env.py
[modify] https://crrev.com/64f2ed4bb2f1a094bba38cb817ed912a118e442e/.vpython
[modify] https://crrev.com/64f2ed4bb2f1a094bba38cb817ed912a118e442e/devil/devil/utils/find_usb_devices_test.py
[modify] https://crrev.com/64f2ed4bb2f1a094bba38cb817ed912a118e442e/telemetry/telemetry/__init__.py
[modify] https://crrev.com/64f2ed4bb2f1a094bba38cb817ed912a118e442e/experimental/bisect_lib/fetch_intervening_revisions_test.py
[modify] https://crrev.com/64f2ed4bb2f1a094bba38cb817ed912a118e442e/common/py_utils/py_utils/cloud_storage_unittest.py
[modify] https://crrev.com/64f2ed4bb2f1a094bba38cb817ed912a118e442e/devil/devil/android/device_utils_test.py
[modify] https://crrev.com/64f2ed4bb2f1a094bba38cb817ed912a118e442e/tracing/bin/run_py_tests
[modify] https://crrev.com/64f2ed4bb2f1a094bba38cb817ed912a118e442e/tracing/PRESUBMIT.py
[modify] https://crrev.com/64f2ed4bb2f1a094bba38cb817ed912a118e442e/tracing/bin/run_dev_server_tests
[modify] https://crrev.com/64f2ed4bb2f1a094bba38cb817ed912a118e442e/common/py_vulcanize/bin/run_py_tests
[modify] https://crrev.com/64f2ed4bb2f1a094bba38cb817ed912a118e442e/devil/devil/devil_env_test.py
[modify] https://crrev.com/64f2ed4bb2f1a094bba38cb817ed912a118e442e/dependency_manager/dependency_manager/base_config_unittest.py
[modify] https://crrev.com/64f2ed4bb2f1a094bba38cb817ed912a118e442e/dependency_manager/dependency_manager/__init__.py
[modify] https://crrev.com/64f2ed4bb2f1a094bba38cb817ed912a118e442e/devil/devil/android/app_ui_test.py
[modify] https://crrev.com/64f2ed4bb2f1a094bba38cb817ed912a118e442e/tracing/bin/run_tests
[modify] https://crrev.com/64f2ed4bb2f1a094bba38cb817ed912a118e442e/devil/devil/utils/mock_calls_test.py
[modify] https://crrev.com/64f2ed4bb2f1a094bba38cb817ed912a118e442e/experimental/PRESUBMIT.py
[add] https://crrev.com/64f2ed4bb2f1a094bba38cb817ed912a118e442e/third_party/mock/mock.py
Project Member

Comment 23 by bugdroid1@chromium.org, Sep 24 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/134fcfe7ed8c4b8c26cc4d2783c894cb233ffdad

commit 134fcfe7ed8c4b8c26cc4d2783c894cb233ffdad
Author: chromium-autoroll <chromium-autoroll@skia-public.iam.gserviceaccount.com>
Date: Mon Sep 24 19:51:33 2018

Roll src/third_party/catapult f19e5d119588..e06567b30fdd (1 commits)

https://chromium.googlesource.com/catapult.git/+log/f19e5d119588..e06567b30fdd


git log f19e5d119588..e06567b30fdd --date=short --no-merges --format='%ad %ae %s'
2018-09-24 jbudorick@chromium.org Add vpython to pinpoint tasks.


Created with:
  gclient setdep -r src/third_party/catapult@e06567b30fdd

The AutoRoll server is located here: https://autoroll.skia.org/r/catapult-autoroll

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

If the roll is causing failures, please contact the current sheriff, who should
be CC'd on the roll, and stop the roller if necessary.

CQ_INCLUDE_TRYBOTS=luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel

BUG=chromium:777865,chromium:878777
TBR=sullivan@chromium.org

Change-Id: I2d23dfdac206d7c569bfbce4d72f05eebded15fd
Reviewed-on: https://chromium-review.googlesource.com/1240510
Reviewed-by: chromium-autoroll <chromium-autoroll@skia-public.iam.gserviceaccount.com>
Commit-Queue: chromium-autoroll <chromium-autoroll@skia-public.iam.gserviceaccount.com>
Cr-Commit-Position: refs/heads/master@{#593648}
[modify] https://crrev.com/134fcfe7ed8c4b8c26cc4d2783c894cb233ffdad/DEPS
Project Member

Comment 24 by bugdroid1@chromium.org, Sep 24 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/276e662dbc2525240c0d3abe456086f11d04843b

commit 276e662dbc2525240c0d3abe456086f11d04843b
Author: chromium-autoroll <chromium-autoroll@skia-public.iam.gserviceaccount.com>
Date: Mon Sep 24 21:35:12 2018

Roll src/third_party/catapult e06567b30fdd..64f2ed4bb2f1 (1 commits)

https://chromium.googlesource.com/catapult.git/+log/e06567b30fdd..64f2ed4bb2f1


git log e06567b30fdd..64f2ed4bb2f1 --date=short --no-merges --format='%ad %ae %s'
2018-09-24 jbudorick@chromium.org Revert "Use vpython and remove vendored pymock."


Created with:
  gclient setdep -r src/third_party/catapult@64f2ed4bb2f1

The AutoRoll server is located here: https://autoroll.skia.org/r/catapult-autoroll

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

If the roll is causing failures, please contact the current sheriff, who should
be CC'd on the roll, and stop the roller if necessary.

CQ_INCLUDE_TRYBOTS=luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel

BUG=chromium:878777,chromium:777865
TBR=sullivan@chromium.org

Change-Id: Ia7a75c175d7efa7f628faabf583f193998b4bb60
Reviewed-on: https://chromium-review.googlesource.com/1240835
Reviewed-by: chromium-autoroll <chromium-autoroll@skia-public.iam.gserviceaccount.com>
Commit-Queue: chromium-autoroll <chromium-autoroll@skia-public.iam.gserviceaccount.com>
Cr-Commit-Position: refs/heads/master@{#593694}
[modify] https://crrev.com/276e662dbc2525240c0d3abe456086f11d04843b/DEPS

Comment 25 by perezju@google.com, Dec 7 

Just noted, did this got re-landed?

Comment 26 by perezju@google.com, Dec 7

Status: Assigned (was: Fixed)
From chat off-thread with John, sounds like the work here is not yet completed.

Comment 27 by ntfschr@chromium.org, Jan 3

Cc: ntfschr@chromium.org
What's the status of this? I've been writing tests with assert_not_called() and assert_called_once() without realizing our version of pymock was so out of date. This is hard to catch, since assert_called_once() (and other methods) would be silent, since they're actually mocked out.

Is it reasonable to fix this by updating pymock? pasko@'s tricks in http://crrev/c/1193825 work, but are very non-obvious.

Comment 28 by perezju@chromium.org, Jan 3

Blockedon: 777865

Comment 29 by jbudorick@chromium.org, Jan 7 

#27: are you looking solely at catapult, or in chromium more generally?

For the former, I need to reland the switch to vpython in catapult, which uses a more up-to-date version of pymock. I'm going to try to do that this quarter, but I'll need to muck around with telemetry to ensure it doesn't horribly break cros again.

For the latter, I have no specific plans.

Comment 30 by ntfschr@chromium.org, Jan 7 

I meant for chromium (and other repos, such as clank).

Glad to hear catapult has a good solution on the way. Would a similar solution work for chromium (I think we already use vpython, but it points to 2.7.13)? Does this ticket also track a solution for chromium?

Comment 31 by jbudorick@chromium.org, Jan 7 

No, this is only for catapult, which uses an older vendored version of pymock. I believe the vpython wheel is more up-to-date. I'm not sure about the chromium vendored version in //third_party/pymock.

Comment 32 by nedngu...@google.com, Jan 7

Cc: -nednguyen@chromium.org

Comment 33 by ntfschr@chromium.org, Jan 7 

Filed issue 919605 to cover general chromium concerns.
►

Sign in to add a comment