Check in cq_stats.py

Check in cq_stats.py

BUG=none

Committed: https://chromium.googlesource.com/infra/infra/+/c2ba1b0e7aa09f6fedf12b58d2630ccd4efab1a1

[Paweł Hajdan Jr., 2015-05-05 15:28:01]

phajdan.jr@chromium.org changed reviewers:
+ pgervais@chromium.org, sergeyberezin@chromium.org, sergiyb@chromium.org

[pgervais, 2015-05-05 16:22:58]

Did a very partial review...

https://chromiumcodereview.appspot.com/1125653004/diff/20001/infra/tools/cq_s...
File infra/tools/cq_stats/test/cq_stats_test.py (right):

https://chromiumcodereview.appspot.com/1125653004/diff/20001/infra/tools/cq_s...
infra/tools/cq_stats/test/cq_stats_test.py:640: # for print_stats().
This looks like useless after the giant assert above.

https://chromiumcodereview.appspot.com/1125653004/diff/20001/infra/tools/cq_s...
infra/tools/cq_stats/test/cq_stats_test.py:924: def test_acquire_stats(self):
Without looking at the content of cq_stats.acquire_stats, I doubt this test is
useful (a lot of mocking + no assert ~= smoke testing the mocks). Please tell me
if I'm wrong.

If it's there just for coverage, add # pragma: no cover instead.

[Sergey Berezin, 2015-05-05 17:06:19]

Thanks, Pawel, for taking this up! Appreciate finishing the job I couldn't get
to in months :-)

A few comments, mostly on the use of expectations. Also, my local test.py didn't
pass, and didn't have 100% coverage. Please run 'git cl try' to make sure you're
not missing anything (maybe, leftover local files?).

Otherwise this looks pretty good.

https://codereview.chromium.org/1125653004/diff/20001/infra/tools/cq_stats/te...
File infra/tools/cq_stats/test/cq_stats_test.py (right):

https://codereview.chromium.org/1125653004/diff/20001/infra/tools/cq_stats/te...
infra/tools/cq_stats/test/cq_stats_test.py:62: def mock(self, obj, member,
mock):
nit: infra.git now depends on testing_support, which includes auto_stub:

from testing_support import auto_stub

class TestCQStats(auto_stub.TestCase):
  ...
  self.mock(...)  # No need to define your own copy of mock.

https://codereview.chromium.org/1125653004/diff/20001/infra/tools/cq_stats/te...
infra/tools/cq_stats/test/cq_stats_test.py:211: self.assertListEqual([
Consider using built-in expectations:

  return data  # instead of assertListEqual(...)

Then "./test.py train infra/tools/cq_stats" would create the list automatically.

https://codereview.chromium.org/1125653004/diff/20001/infra/tools/cq_stats/te...
infra/tools/cq_stats/test/cq_stats_test.py:243: self.assertDictEqual({
Likewise, consider using expectations. For both values in one test:

  return (data, derived_data)

https://codereview.chromium.org/1125653004/diff/20001/infra/tools/cq_stats/te...
infra/tools/cq_stats/test/cq_stats_test.py:304: self.assertListEqual([
Consider using expectations.

https://codereview.chromium.org/1125653004/diff/20001/infra/tools/cq_stats/te...
infra/tools/cq_stats/test/cq_stats_test.py:333: self.assertDictEqual({
Consider using expectations.

https://codereview.chromium.org/1125653004/diff/20001/infra/tools/cq_stats/te...
infra/tools/cq_stats/test/cq_stats_test.py:418: self.assertDictEqual({
This definitely needs to go into expectations - it's way too long for a test
code, and will be hard to maintain manually.

https://codereview.chromium.org/1125653004/diff/20001/infra/tools/cq_stats/te...
infra/tools/cq_stats/test/cq_stats_test.py:650: self.assertDictEqual({
Beware of dangers of float arithmetic - it's not exactly the same on different
processors, and may differ in the last digit. The original test was basically a
smoke test (no need to verify numpy correctness). However, if you want to
compare the actual values, consider rounding them in the code (2 places after
the decimal point are probably more than enough).

https://codereview.chromium.org/1125653004/diff/20001/infra/tools/cq_stats/te...
infra/tools/cq_stats/test/cq_stats_test.py:871: item_name=None, committed=False)
  return self.expectations

Otherwise you're not testing the print output.

https://codereview.chromium.org/1125653004/diff/20001/infra/tools/cq_stats/te...
infra/tools/cq_stats/test/cq_stats_test.py:875: 
nit: stray spaces on empty line.

https://codereview.chromium.org/1125653004/diff/20001/infra/tools/cq_stats/te...
infra/tools/cq_stats/test/cq_stats_test.py:876: cq_stats.print_duration('mean',
Args(), cq_stats.default_stats(), None)
  return self.expectations

https://codereview.chromium.org/1125653004/diff/20001/infra/tools/cq_stats/te...
infra/tools/cq_stats/test/cq_stats_test.py:886: cq_stats.print_usage(Args(),
stats, stats)
  return self.expectations

https://codereview.chromium.org/1125653004/diff/20001/infra/tools/cq_stats/te...
infra/tools/cq_stats/test/cq_stats_test.py:922: cq_stats.print_stats(args,
{'latest': stats_set, 'previous': stats_set})
  return self.expectations

https://codereview.chromium.org/1125653004/diff/20001/infra/tools/cq_stats/te...
infra/tools/cq_stats/test/cq_stats_test.py:924: def test_acquire_stats(self):
On 2015/05/05 16:22:58, pgervais wrote:
> Without looking at the content of cq_stats.acquire_stats, I doubt this test is
> useful (a lot of mocking + no assert ~= smoke testing the mocks). Please tell
me
> if I'm wrong.
> 
> If it's there just for coverage, add # pragma: no cover instead. 

I disagree with #pragma: no cover - a smoke test that executes code is better
than no test at all, and "no cover" prevents automatic coverage enforcement.

I'm actually OK with it being a smoke test, since all the other functions are
independently tested. It would be a more comprehensive integration(-ish) test if
mocks were at least one level deeper, so the API compatibility is actually
tested, but I don't know how easy it is to achieve.

https://codereview.chromium.org/1125653004/diff/20001/infra/tools/cq_stats/te...
infra/tools/cq_stats/test/cq_stats_test.py:950: cq_stats.main()
  return self.expectations

[pgervais, 2015-05-05 21:34:05]

https://codereview.chromium.org/1125653004/diff/20001/infra/tools/cq_stats/te...
File infra/tools/cq_stats/test/cq_stats_test.py (right):

https://codereview.chromium.org/1125653004/diff/20001/infra/tools/cq_stats/te...
infra/tools/cq_stats/test/cq_stats_test.py:924: def test_acquire_stats(self):
On 2015/05/05 17:06:19, Sergey Berezin wrote:
> On 2015/05/05 16:22:58, pgervais wrote:
> > Without looking at the content of cq_stats.acquire_stats, I doubt this test
is
> > useful (a lot of mocking + no assert ~= smoke testing the mocks). Please
tell
> me
> > if I'm wrong.
> > 
> > If it's there just for coverage, add # pragma: no cover instead. 
> 
> I disagree with #pragma: no cover - a smoke test that executes code is better
> than no test at all, and "no cover" prevents automatic coverage enforcement.
> 
> I'm actually OK with it being a smoke test, since all the other functions are
> independently tested. It would be a more comprehensive integration(-ish) test
if
> mocks were at least one level deeper, so the API compatibility is actually
> tested, but I don't know how easy it is to achieve.

What I meant is that with that amount of mocking, even a smoke test is kind of
useless. Whatever...

[Sergiy Byelozyorov, 2015-05-06 09:56:58]

I am not sure if Git correctly handles empty files. Please put a copyright
header into both __init__.py.

[Paweł Hajdan Jr., 2015-05-06 11:09:16]

Thanks for the review.

For now I'm keeping __init__.py files empty, consistent with other files in this
repo. I'm not aware of any problems with git and empty files (it doesn't track
empty directories though - not applicable to this CL). Could you explain more,
Sergiy?

https://codereview.chromium.org/1125653004/diff/20001/infra/tools/cq_stats/te...
File infra/tools/cq_stats/test/cq_stats_test.py (right):

https://codereview.chromium.org/1125653004/diff/20001/infra/tools/cq_stats/te...
infra/tools/cq_stats/test/cq_stats_test.py:62: def mock(self, obj, member,
mock):
On 2015/05/05 at 17:06:18, Sergey Berezin wrote:
> nit: infra.git now depends on testing_support, which includes auto_stub:
> 
> from testing_support import auto_stub
> 
> class TestCQStats(auto_stub.TestCase):
>   ...
>   self.mock(...)  # No need to define your own copy of mock.

Done.

https://codereview.chromium.org/1125653004/diff/20001/infra/tools/cq_stats/te...
infra/tools/cq_stats/test/cq_stats_test.py:211: self.assertListEqual([
On 2015/05/05 at 17:06:18, Sergey Berezin wrote:
> Consider using built-in expectations:
> 
>   return data  # instead of assertListEqual(...)
> 
> Then "./test.py train infra/tools/cq_stats" would create the list
automatically.

Note there are tricky issues with datetime here. Do you have suggestions how to
handle it e.g. on the expect_tests side?

Traceback (most recent call last):
  File "/usr/lib/python2.7/multiprocessing/process.py", line 258, in _bootstrap
    self.run()
  File "/usr/lib/python2.7/multiprocessing/process.py", line 114, in run
    self._target(*self._args, **self._kwargs)
  File
"/usr/local/google/home/phajdan/infra/infra/ENV/local/lib/python2.7/site-packages/expect_tests/pipeline.py",
line 373, in run_loop_process
    result_queue.put_nowait)
  File
"/usr/local/google/home/phajdan/infra/infra/ENV/local/lib/python2.7/site-packages/expect_tests/handle_train.py",
line 66, in run_stage_loop
    WriteNewData(test, result.data)
  File
"/usr/local/google/home/phajdan/infra/infra/ENV/local/lib/python2.7/site-packages/expect_tests/serialize.py",
line 107, in WriteNewData
    SERIALIZERS[test.ext][1](data, f)
  File
"/usr/local/google/home/phajdan/infra/infra/ENV/local/lib/python2.7/site-packages/expect_tests/serialize.py",
line 40, in <lambda>
    data, stream, sort_keys=True, indent=2, separators=(',', ': ')))
  File "/usr/lib/python2.7/json/__init__.py", line 189, in dump
    for chunk in iterable:
  File "/usr/lib/python2.7/json/encoder.py", line 431, in _iterencode
    for chunk in _iterencode_list(o, _current_indent_level):
  File "/usr/lib/python2.7/json/encoder.py", line 332, in _iterencode_list
    for chunk in chunks:
  File "/usr/lib/python2.7/json/encoder.py", line 332, in _iterencode_list
    for chunk in chunks:
  File "/usr/lib/python2.7/json/encoder.py", line 408, in _iterencode_dict
    for chunk in chunks:
  File "/usr/lib/python2.7/json/encoder.py", line 442, in _iterencode
    o = _default(o)
  File "/usr/lib/python2.7/json/encoder.py", line 184, in default
    raise TypeError(repr(o) + " is not JSON serializable")
TypeError: datetime.datetime(2014, 11, 23, 22, 38, 39) is not JSON serializable

https://codereview.chromium.org/1125653004/diff/20001/infra/tools/cq_stats/te...
infra/tools/cq_stats/test/cq_stats_test.py:650: self.assertDictEqual({
On 2015/05/05 at 17:06:19, Sergey Berezin wrote:
> Beware of dangers of float arithmetic - it's not exactly the same on different
processors, and may differ in the last digit. The original test was basically a
smoke test (no need to verify numpy correctness). However, if you want to
compare the actual values, consider rounding them in the code (2 places after
the decimal point are probably more than enough).

Done.

https://codereview.chromium.org/1125653004/diff/20001/infra/tools/cq_stats/te...
infra/tools/cq_stats/test/cq_stats_test.py:871: item_name=None, committed=False)
On 2015/05/05 at 17:06:18, Sergey Berezin wrote:
> return self.expectations
> 
> Otherwise you're not testing the print output.

Done.

https://codereview.chromium.org/1125653004/diff/20001/infra/tools/cq_stats/te...
infra/tools/cq_stats/test/cq_stats_test.py:875: 
On 2015/05/05 at 17:06:19, Sergey Berezin wrote:
> nit: stray spaces on empty line.

Done.

https://codereview.chromium.org/1125653004/diff/20001/infra/tools/cq_stats/te...
infra/tools/cq_stats/test/cq_stats_test.py:876: cq_stats.print_duration('mean',
Args(), cq_stats.default_stats(), None)
On 2015/05/05 at 17:06:18, Sergey Berezin wrote:
> return self.expectations

Done.

https://codereview.chromium.org/1125653004/diff/20001/infra/tools/cq_stats/te...
infra/tools/cq_stats/test/cq_stats_test.py:886: cq_stats.print_usage(Args(),
stats, stats)
On 2015/05/05 at 17:06:18, Sergey Berezin wrote:
> return self.expectations

Done.

https://codereview.chromium.org/1125653004/diff/20001/infra/tools/cq_stats/te...
infra/tools/cq_stats/test/cq_stats_test.py:922: cq_stats.print_stats(args,
{'latest': stats_set, 'previous': stats_set})
On 2015/05/05 at 17:06:18, Sergey Berezin wrote:
> return self.expectations

Done.

https://codereview.chromium.org/1125653004/diff/20001/infra/tools/cq_stats/te...
infra/tools/cq_stats/test/cq_stats_test.py:924: def test_acquire_stats(self):
On 2015/05/05 at 21:34:04, pgervais wrote:
> On 2015/05/05 17:06:19, Sergey Berezin wrote:
> > On 2015/05/05 16:22:58, pgervais wrote:
> > > Without looking at the content of cq_stats.acquire_stats, I doubt this
test is
> > > useful (a lot of mocking + no assert ~= smoke testing the mocks). Please
tell
> > me
> > > if I'm wrong.
> > > 
> > > If it's there just for coverage, add # pragma: no cover instead. 
> > 
> > I disagree with #pragma: no cover - a smoke test that executes code is
better
> > than no test at all, and "no cover" prevents automatic coverage enforcement.
> > 
> > I'm actually OK with it being a smoke test, since all the other functions
are
> > independently tested. It would be a more comprehensive integration(-ish)
test if
> > mocks were at least one level deeper, so the API compatibility is actually
> > tested, but I don't know how easy it is to achieve.
> 
> What I meant is that with that amount of mocking, even a smoke test is kind of
useless. Whatever...

I agree the usefulness of this test is limited. I see at least the following
choices at this moment:

1) keep the code as-is (i.e. just a smoke test)
2) remove the smoke test and add "pragma: no cover" statements; it seems however
we're getting at least "crash coverage" as Sergey mentioned
3) change both test code and code under test so that we can exercise the logic
without mocking so much (for now I could also add a TODO for that)

What are your recommendations?

https://codereview.chromium.org/1125653004/diff/20001/infra/tools/cq_stats/te...
infra/tools/cq_stats/test/cq_stats_test.py:950: cq_stats.main()
On 2015/05/05 at 17:06:18, Sergey Berezin wrote:
> return self.expectations

Done.

[Sergiy Byelozyorov, 2015-05-06 11:21:45]

On 2015/05/06 11:09:16, Paweł Hajdan Jr. wrote:
> For now I'm keeping __init__.py files empty, consistent with other files in
this
> repo. I'm not aware of any problems with git and empty files (it doesn't track
> empty directories though - not applicable to this CL). Could you explain more,
> Sergiy?

Oh indeed. I confused empty files and empty directories. Nevermind then.
I've did a scan over the code, but didn't actually review any of it.
Please let me know if you want me to do a thorough review.

[pgervais, 2015-05-06 20:20:07]

On 2015/05/06 11:21:45, Sergiy Byelozyorov wrote:
> On 2015/05/06 11:09:16, Paweł Hajdan Jr. wrote:
> > For now I'm keeping __init__.py files empty, consistent with other files in
> this
> > repo. I'm not aware of any problems with git and empty files (it doesn't
track
> > empty directories though - not applicable to this CL). Could you explain
more,
> > Sergiy?
> 
> Oh indeed. I confused empty files and empty directories. Nevermind then.
> I've did a scan over the code, but didn't actually review any of it.
> Please let me know if you want me to do a thorough review.

The problem is not with git, it's with the way rietveld communiates with the try
server. The tryserver apparently has no way to tell the difference between an
empty file and a deleted or non-existing file. The net result is that when you
add a new Python package, the __init__.py is not created if it's empty, and the
package is missing. Or worse, no tests are run and the build is green. I added
the presubmit test that checks for empty files for this very reason. 

What it means for the present CL is that no tests will be run for cq_stats on
the tryjob, and it may break on the continuous builder afterward.

[pgervais, 2015-05-07 00:37:21]

https://codereview.chromium.org/1125653004/diff/20001/infra/tools/cq_stats/te...
File infra/tools/cq_stats/test/cq_stats_test.py (right):

https://codereview.chromium.org/1125653004/diff/20001/infra/tools/cq_stats/te...
infra/tools/cq_stats/test/cq_stats_test.py:211: self.assertListEqual([
On 2015/05/06 11:09:15, Paweł Hajdan Jr. wrote:
> On 2015/05/05 at 17:06:18, Sergey Berezin wrote:
> > Consider using built-in expectations:
> > 
> >   return data  # instead of assertListEqual(...)
> > 
> > Then "./test.py train infra/tools/cq_stats" would create the list
> automatically.
> 
> Note there are tricky issues with datetime here. Do you have suggestions how
to
> handle it e.g. on the expect_tests side?
> 
> Traceback (most recent call last):
>   File "/usr/lib/python2.7/multiprocessing/process.py", line 258, in
_bootstrap
>     self.run()
>   File "/usr/lib/python2.7/multiprocessing/process.py", line 114, in run
>     self._target(*self._args, **self._kwargs)
>   File
>
"/usr/local/google/home/phajdan/infra/infra/ENV/local/lib/python2.7/site-packages/expect_tests/pipeline.py",
> line 373, in run_loop_process
>     result_queue.put_nowait)
>   File
>
"/usr/local/google/home/phajdan/infra/infra/ENV/local/lib/python2.7/site-packages/expect_tests/handle_train.py",
> line 66, in run_stage_loop
>     WriteNewData(test, result.data)
>   File
>
"/usr/local/google/home/phajdan/infra/infra/ENV/local/lib/python2.7/site-packages/expect_tests/serialize.py",
> line 107, in WriteNewData
>     SERIALIZERS[test.ext][1](data, f)
>   File
>
"/usr/local/google/home/phajdan/infra/infra/ENV/local/lib/python2.7/site-packages/expect_tests/serialize.py",
> line 40, in <lambda>
>     data, stream, sort_keys=True, indent=2, separators=(',', ': ')))
>   File "/usr/lib/python2.7/json/__init__.py", line 189, in dump
>     for chunk in iterable:
>   File "/usr/lib/python2.7/json/encoder.py", line 431, in _iterencode
>     for chunk in _iterencode_list(o, _current_indent_level):
>   File "/usr/lib/python2.7/json/encoder.py", line 332, in _iterencode_list
>     for chunk in chunks:
>   File "/usr/lib/python2.7/json/encoder.py", line 332, in _iterencode_list
>     for chunk in chunks:
>   File "/usr/lib/python2.7/json/encoder.py", line 408, in _iterencode_dict
>     for chunk in chunks:
>   File "/usr/lib/python2.7/json/encoder.py", line 442, in _iterencode
>     o = _default(o)
>   File "/usr/lib/python2.7/json/encoder.py", line 184, in default
>     raise TypeError(repr(o) + " is not JSON serializable")
> TypeError: datetime.datetime(2014, 11, 23, 22, 38, 39) is not JSON
serializable

I you serialize the datetime here in the test function, then you can use the
built-in expectation system.

[Paweł Hajdan Jr., 2015-05-07 11:08:55]

PTAL

I'm happy to take further specific actionable suggestions what to do with this
patch.

If possible let's move though towards landing it (or decreasing number of
round-trips).

[pgervais, 2015-05-07 15:42:23]

The new way to handle expectations looks good to me, leaving the final decision
to Sergey.

[Sergey Berezin, 2015-05-07 16:51:16]

LGTM, thanks for figuring out the expectations.

I also filed a bug: http://crbug.com/485373, soon to be resolved. But no need to
block this CL on it.

[Paweł Hajdan Jr., 2015-05-08 09:49:05]

The CQ bit was checked by phajdan.jr@chromium.org

[commit-bot: I haz the power, 2015-05-08 09:49:17]

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

[commit-bot: I haz the power, 2015-05-08 09:57:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-05-08 09:57:02]

Try jobs failed on following builders:
  infra_tester on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/infra_tester/bu...)

[Paweł Hajdan Jr., 2015-05-08 11:30:33]

The CQ bit was checked by phajdan.jr@chromium.org

[Paweł Hajdan Jr., 2015-05-08 11:30:33]

The patchset sent to the CQ was uploaded after l-g-t-m from
sergeyberezin@chromium.org
Link to the patchset: https://codereview.chromium.org/1125653004/#ps100001
(title: "use utc")

[commit-bot: I haz the power, 2015-05-08 11:30:50]

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

[commit-bot: I haz the power, 2015-05-08 11:39:22]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/infra/infra/+/c2ba1b0e7aa09f6fedf12b58d2630...