Format profiler/trace files cloud url

Format profiler/trace files cloud url

This CL adds ProfilerValue which holds the cloud url for the profiler files
and can be added using results.AddValue(ProfilerValue()).

When doing format output, ProfilerValue and TraceValue will put their
cloud url in the key named "cloud_url".

BUG=431904
TEST= see #8 at crbug.com/431904

Committed: https://crrev.com/69cc6288af065c0919e0e59e5817c73705f315b3
Cr-Commit-Position: refs/heads/master@{#305071}

[Yufeng Shen (Slow to review), 2014-11-19 23:02:34]

miletus@chromium.org changed reviewers:
+ nednguyen@google.com, qyearsley@chromium.org, sullivan@chromium.org

[Yufeng Shen (Slow to review), 2014-11-19 23:03:18]

I posted the sample chartjson output at #8 of crbug.com/431904

Let me know if the format makes sense.

[nednguyen, 2014-11-19 23:31:09]

On 2014/11/19 23:03:18, Yufeng Shen wrote:
> I posted the sample chartjson output at #8 of crbug.com/431904
> 
> Let me know if the format makes sense.

Can you add unittest for these new logic? I realize that the last two patches
weren't have new unittest coverage.

[miletus1, 2014-11-20 01:45:17]

On 2014/11/19 23:31:09, nednguyen wrote:
> On 2014/11/19 23:03:18, Yufeng Shen wrote:
> > I posted the sample chartjson output at #8 of crbug.com/431904
> > 
> > Let me know if the format makes sense.
> 
> Can you add unittest for these new logic? I realize that the last two patches
> weren't have new unittest coverage.

Done. Let me know if more test coverage is needed.

[nednguyen, 2014-11-20 02:04:11]

Please don't add profiler_values into the value system. Profiler is supposed to
be moved intro TraceValue later on, so let make it a special case just like in
your original patch.

https://codereview.chromium.org/731233003/diff/60001/tools/perf/benchmarks/be...
File tools/perf/benchmarks/benchmark_smoke_unittest.py (right):

https://codereview.chromium.org/731233003/diff/60001/tools/perf/benchmarks/be...
tools/perf/benchmarks/benchmark_smoke_unittest.py:50: options.upload_results =
True
We will revert these changes upon submission, I assume?

https://codereview.chromium.org/731233003/diff/60001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/results/page_test_results.py (right):

https://codereview.chromium.org/731233003/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/results/page_test_results.py:179: def
FindAllTraceValue(self):
FindAllTraceValues

https://codereview.chromium.org/731233003/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/results/page_test_results.py:186: def
FindAllProfilerValue(self):
FindAllProfilerValues

https://codereview.chromium.org/731233003/diff/60001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/results/page_test_results_unittest.py (right):

https://codereview.chromium.org/731233003/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/results/page_test_results_unittest.py:189: def
testTraceAndProfilerValue(self):
Please split this into two tests: one for trace & and one for profiler values.

https://codereview.chromium.org/731233003/diff/60001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/value/trace.py (right):

https://codereview.chromium.org/731233003/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/trace.py:109: self._cloud_url = 'error'
I would rather not set _cloud_url.

https://codereview.chromium.org/731233003/diff/60001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/value/trace_unittest.py (right):

https://codereview.chromium.org/731233003/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/trace_unittest.py:39:
self.assertTrue(v.cloud_url)
This test seems to does a network push anytime you run the test. Can you fake
out the cloud storage so you can also test the cloud_url & permission error
case?

You can do thing like:
trace.cloud_storage = MyFakeCloudStorage()

[Yufeng Shen (Slow to review), 2014-11-20 17:56:16]

Removed the profiler part and now only the TraceValue is
formatted.

https://codereview.chromium.org/731233003/diff/60001/tools/perf/benchmarks/be...
File tools/perf/benchmarks/benchmark_smoke_unittest.py (right):

https://codereview.chromium.org/731233003/diff/60001/tools/perf/benchmarks/be...
tools/perf/benchmarks/benchmark_smoke_unittest.py:50: options.upload_results =
True
On 2014/11/20 02:04:11, nednguyen wrote:
> We will revert these changes upon submission, I assume?

yes.

https://codereview.chromium.org/731233003/diff/60001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/results/page_test_results.py (right):

https://codereview.chromium.org/731233003/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/results/page_test_results.py:179: def
FindAllTraceValue(self):
On 2014/11/20 02:04:11, nednguyen wrote:
> FindAllTraceValues

Done.

https://codereview.chromium.org/731233003/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/results/page_test_results.py:186: def
FindAllProfilerValue(self):
On 2014/11/20 02:04:11, nednguyen wrote:
> FindAllProfilerValues

removed.

https://codereview.chromium.org/731233003/diff/60001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/results/page_test_results_unittest.py (right):

https://codereview.chromium.org/731233003/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/results/page_test_results_unittest.py:189: def
testTraceAndProfilerValue(self):
On 2014/11/20 02:04:11, nednguyen wrote:
> Please split this into two tests: one for trace & and one for profiler values.

Only test TraceValue now

https://codereview.chromium.org/731233003/diff/60001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/value/trace.py (right):

https://codereview.chromium.org/731233003/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/trace.py:109: self._cloud_url = 'error'
On 2014/11/20 02:04:11, nednguyen wrote:
> I would rather not set _cloud_url.

Done.

https://codereview.chromium.org/731233003/diff/60001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/value/trace_unittest.py (right):

https://codereview.chromium.org/731233003/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/trace_unittest.py:39:
self.assertTrue(v.cloud_url)
On 2014/11/20 02:04:11, nednguyen wrote:
> This test seems to does a network push anytime you run the test. Can you fake
> out the cloud storage so you can also test the cloud_url & permission error
> case?
> 
> You can do thing like:
> trace.cloud_storage = MyFakeCloudStorage()

changed to use system_stub

[nednguyen, 2014-11-20 18:46:54]

lgtm

https://codereview.chromium.org/731233003/diff/80001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/value/trace.py (right):

https://codereview.chromium.org/731233003/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/trace.py:54: def cloud_url(self):
This doesn't need to be exposed. If pylint complains about the ._cloud_url
access in unittest, just suppress it.

https://codereview.chromium.org/731233003/diff/80001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/value/trace_unittest.py (right):

https://codereview.chromium.org/731233003/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/trace_unittest.py:27: def
StubCloudStorage(self):
just make this setUp(self):

https://codereview.chromium.org/731233003/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/trace_unittest.py:41: {fh.GetAbsPath(): None, })
Can you use some random value rather than None?

[Yufeng Shen (Slow to review), 2014-11-20 19:13:35]

https://codereview.chromium.org/731233003/diff/80001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/value/trace.py (right):

https://codereview.chromium.org/731233003/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/trace.py:54: def cloud_url(self):
On 2014/11/20 18:46:53, nednguyen wrote:
> This doesn't need to be exposed. If pylint complains about the ._cloud_url
> access in unittest, just suppress it.

Done.

https://codereview.chromium.org/731233003/diff/80001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/value/trace_unittest.py (right):

https://codereview.chromium.org/731233003/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/trace_unittest.py:27: def
StubCloudStorage(self):
On 2014/11/20 18:46:53, nednguyen wrote:
> just make this setUp(self):

Done.

https://codereview.chromium.org/731233003/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/trace_unittest.py:41: {fh.GetAbsPath(): None, })
On 2014/11/20 18:46:53, nednguyen wrote:
> Can you use some random value rather than None?

Done.

[Yufeng Shen (Slow to review), 2014-11-20 19:15:46]

The CQ bit was checked by miletus@chromium.org

[commit-bot: I haz the power, 2014-11-20 19:17:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/731233003/120001

[commit-bot: I haz the power, 2014-11-20 20:17:36]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2014-11-20 20:18:13]

Patchset 7 (id:??) landed as
https://crrev.com/69cc6288af065c0919e0e59e5817c73705f315b3
Cr-Commit-Position: refs/heads/master@{#305071}