Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(106)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 2889583003: [Telemetry][CodeHealth] Make unittests with fake platforms use fakes.FakePlatform (Closed)

Created:
3 years, 7 months ago by rnephew (Reviews Here)
Modified:
3 years, 7 months ago
CC:
catapult-reviews_chromium.org, telemetry-reviews_chromium.org
Target Ref:
refs/heads/master
Project:
catapult
Visibility:
Public.

Description

Patch Set 1 #

Total comments: 2

Patch Set 2 : fix variable names for unused variables #

Total comments: 2

Patch Set 3 : [Telemetry][CodeHealth] Make unittests with fake platforms use fakes.FakePlatform #

Unified diffs Side-by-side diffs Delta from patch set Stats (+40 lines, -76 lines) Patch
M telemetry/telemetry/internal/browser/tab_unittest.py View 3 chunks +2 lines, -31 lines 0 comments Download
M telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent_unittest.py View 4 chunks +8 lines, -28 lines 0 comments Download
M telemetry/telemetry/internal/story_runner_unittest.py View 3 chunks +2 lines, -16 lines 0 comments Download
M telemetry/telemetry/testing/fakes/__init__.py View 1 2 3 chunks +28 lines, -1 line 0 comments Download

Messages

Total messages: 17 (6 generated)
rnephew (Reviews Here)
3 years, 7 months ago (2017-05-16 17:31:06 UTC) #3
nednguyen
Awesome, thanks Randy! https://codereview.chromium.org/2889583003/diff/20001/telemetry/telemetry/testing/fakes/__init__.py File telemetry/telemetry/testing/fakes/__init__.py (right): https://codereview.chromium.org/2889583003/diff/20001/telemetry/telemetry/testing/fakes/__init__.py#newcode32 telemetry/telemetry/testing/fakes/__init__.py:32: def DidStartBrowser(self, _, _2): nits: our ...
3 years, 7 months ago (2017-05-16 17:39:26 UTC) #4
rnephew (Reviews Here)
https://codereview.chromium.org/2889583003/diff/20001/telemetry/telemetry/testing/fakes/__init__.py File telemetry/telemetry/testing/fakes/__init__.py (right): https://codereview.chromium.org/2889583003/diff/20001/telemetry/telemetry/testing/fakes/__init__.py#newcode32 telemetry/telemetry/testing/fakes/__init__.py:32: def DidStartBrowser(self, _, _2): On 2017/05/16 17:39:25, nednguyen wrote: ...
3 years, 7 months ago (2017-05-16 17:56:28 UTC) #5
nednguyen
lgtm https://codereview.chromium.org/2889583003/diff/40001/telemetry/telemetry/testing/fakes/__init__.py File telemetry/telemetry/testing/fakes/__init__.py (right): https://codereview.chromium.org/2889583003/diff/40001/telemetry/telemetry/testing/fakes/__init__.py#newcode33 telemetry/telemetry/testing/fakes/__init__.py:33: del browser # unused nits: you can do ...
3 years, 7 months ago (2017-05-16 17:59:10 UTC) #6
rnephew (Reviews Here)
https://codereview.chromium.org/2889583003/diff/40001/telemetry/telemetry/testing/fakes/__init__.py File telemetry/telemetry/testing/fakes/__init__.py (right): https://codereview.chromium.org/2889583003/diff/40001/telemetry/telemetry/testing/fakes/__init__.py#newcode33 telemetry/telemetry/testing/fakes/__init__.py:33: del browser # unused On 2017/05/16 17:59:10, nednguyen wrote: ...
3 years, 7 months ago (2017-05-16 18:02:20 UTC) #7
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/2889583003/60001
3 years, 7 months ago (2017-05-16 18:02:27 UTC) #10
commit-bot: I haz the power
Committed patchset #3 (id:60001) as https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/8b1e70dd66320bb84b8d7efe30016ea0e5023097
3 years, 7 months ago (2017-05-16 18:30:44 UTC) #13
rnephew (Reviews Here)
I think this may have introduced some sort of strange circular dependency... I will be ...
3 years, 7 months ago (2017-05-16 20:33:08 UTC) #14
rnephew (Reviews Here)
A revert of this CL (patchset #3 id:60001) has been created in https://codereview.chromium.org/2887963002/ by rnephew@chromium.org. ...
3 years, 7 months ago (2017-05-16 20:33:28 UTC) #15
rnephew (Reviews Here)
On 2017/05/16 20:33:08, rnephew (Reviews Here) wrote: > I think this may have introduced some ...
3 years, 7 months ago (2017-05-16 20:44:12 UTC) #16
nednguyen
3 years, 7 months ago (2017-05-16 21:03:05 UTC) #17
Message was sent while issue was closed. 
On 2017/05/16 20:44:12, rnephew (Reviews Here) wrote:
> On 2017/05/16 20:33:08, rnephew (Reviews Here) wrote:
> > I think this may have introduced some sort of strange circular dependency...
I
> > will be reverting it.
> > 
> > Traceback (most recent call last):
> >   <module> at
> >
/usr/local/google/home/rnephew/chromium/clank2/src/tools/perf/run_benchmark:9
> >     from core import trybot_command
> >   <module> at
> >
>
/usr/local/google/home/rnephew/chromium/clank2/src/tools/perf/core/trybot_command.py:19
> >     from core import benchmark_finders
> >   <module> at
> >
>
/usr/local/google/home/rnephew/chromium/clank2/src/tools/perf/core/benchmark_finders.py:10
> >     from core import perf_benchmark
> >   <module> at
> >
>
/usr/local/google/home/rnephew/chromium/clank2/src/tools/perf/core/perf_benchmark.py:8
> >     from telemetry import benchmark
> >   <module> at
> >
>
/usr/local/google/home/rnephew/chromium/clank2/src/third_party/catapult/telemetry/telemetry/benchmark.py:8
> >     from telemetry.internal import story_runner
> >   <module> at
> >
>
/usr/local/google/home/rnephew/chromium/clank2/src/third_party/catapult/telemetry/telemetry/internal/story_runner.py:20
> >     from telemetry import page
> >   <module> at
> >
>
/usr/local/google/home/rnephew/chromium/clank2/src/third_party/catapult/telemetry/telemetry/page/__init__.py:13
> >     from telemetry.page import shared_page_state
> >   <module> at
> >
>
/usr/local/google/home/rnephew/chromium/clank2/src/third_party/catapult/telemetry/telemetry/page/shared_page_state.py:331
> >     class SharedDesktopPageState(SharedPageState):
> >   SharedDesktopPageState at
> >
>
/usr/local/google/home/rnephew/chromium/clank2/src/third_party/catapult/telemetry/telemetry/page/shared_page_state.py:332
> >     if platform_module.GetHostPlatform().GetOSName() == 'chromeos':
> >   GetHostPlatform at
> >
>
/usr/local/google/home/rnephew/chromium/clank2/src/third_party/catapult/telemetry/telemetry/core/platform.py:38
> >     _InitHostPlatformIfNeeded()
> >   _InitHostPlatformIfNeeded at
> >
>
/usr/local/google/home/rnephew/chromium/clank2/src/third_party/catapult/telemetry/telemetry/core/platform.py:27
> >     backends = _IterAllPlatformBackendClasses()
> >   _IterAllPlatformBackendClasses at
> >
>
/usr/local/google/home/rnephew/chromium/clank2/src/third_party/catapult/telemetry/telemetry/core/platform.py:47
> >     platform_backend_module.PlatformBackend).itervalues()
> >   DiscoverClasses at
> >
>
/usr/local/google/home/rnephew/chromium/clank2/src/third_party/catapult/telemetry/telemetry/core/discover.py:99
> >     modules = DiscoverModules(start_dir, top_level_dir, pattern)
> >   DiscoverModules at
> >
>
/usr/local/google/home/rnephew/chromium/clank2/src/third_party/catapult/telemetry/telemetry/core/discover.py:58
> >     module = __import__(module_name, fromlist=[True])
> >   <module> at
> >
>
/usr/local/google/home/rnephew/chromium/clank2/src/third_party/catapult/telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent_unittest.py:11
> >     from telemetry.testing import fakes
> >   <module> at
> >
>
/usr/local/google/home/rnephew/chromium/clank2/src/third_party/catapult/telemetry/telemetry/testing/fakes/__init__.py:19
> >     from telemetry.page import shared_page_state
> > ImportError: cannot import name shared_page_state
> 
> It looks like the discover module doesn't play nice, and is causing strange
> import patterns. Might not be able to use the fake module in any place where
the
> discover module is also used :/
fakes/__init__.py should not depend on shared_page_state

Powered by Google App Engine
This is Rietveld 408576698