Re: Add usage message with available page_sets and benchmarks. (issue 432543003 by ariblue@google.com)

4 views
Skip to first unread message

nedn...@google.com

unread,
Apr 27, 2015, 2:27:56 PM4/27/15
to ari...@google.com, chris...@google.com, d...@chromium.org, chromium...@chromium.org, telemet...@chromium.org
On 2014/09/04 03:31:37, nednguyen wrote:
> Can you also add unittest for new functions in discover.py?


https://codereview.chromium.org/432543003/diff/40001/tools/telemetry/telemetry/core/discover.py
> File tools/telemetry/telemetry/core/discover.py (right):


https://codereview.chromium.org/432543003/diff/40001/tools/telemetry/telemetry/core/discover.py#newcode18
> tools/telemetry/telemetry/core/discover.py:18: config =
> environment.Environment([util.GetBaseDir()])
> Why do we need a global?


https://codereview.chromium.org/432543003/diff/40001/tools/telemetry/telemetry/core/discover.py#newcode144
> tools/telemetry/telemetry/core/discover.py:144: def
> GetValidSubclassesOfClasses(base_classes, base_dir=None):
> Please add docstring for this function and the ones below.


https://codereview.chromium.org/432543003/diff/40001/tools/telemetry/telemetry/core/discover.py#newcode183
> tools/telemetry/telemetry/core/discover.py:183: output_stream=sys.stdout):
> Can we split this function to two functions: one that prints class in
> classes_list in sorted order, and one that finds subclasses of
> base_classes?

> And do we really have the use cases on the call sites that need the 2nd
> function?


https://codereview.chromium.org/432543003/diff/40001/tools/telemetry/telemetry/test_runner.py
> File tools/telemetry/telemetry/test_runner.py (right):


https://codereview.chromium.org/432543003/diff/40001/tools/telemetry/telemetry/test_runner.py#newcode27
> tools/telemetry/telemetry/test_runner.py:27: test_class_types =
> [benchmark.Benchmark, page_test.PageTest]
> May better be explicit about this: _BENCHMARK_AND_PAGE_TESTS_CLASSES

Dave, maybe this patch still has a lot of relevant idea for fixing wpr.

https://codereview.chromium.org/432543003/
Reply all
Reply to author
Forward
0 new messages