Tests formatted as per PEP8 guidelines #227

pravarag · 2018-12-20T19:25:51Z

Formatted test scripts for jaeger-client-python as per the PEP8 guidelines for better readability.

black-adder · 2018-12-20T19:49:57Z

Was this done manually or did you run a linter? If the latter, it might be better to add that to the makefile and travis scripts so that the linter can run automatically. Also, I think you have to adjust the rules of the linter as the license headers should be comments, not a string literal.

pravarag · 2018-12-21T05:14:09Z

For now I've done manually only. But adding a linter is a good suggestion. I'll add that surely then...

yurishkuro · 2018-12-28T23:53:07Z

tests/test_TUDPTransport.py

-# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-# See the License for the specific language governing permissions and
-# limitations under the License.
+"""


copyright header is not a docs string, it should remain as comments,

yurishkuro · 2018-12-29T00:00:02Z

not opposed to merging this, but need to address a couple of things:

undo changes to the license headers
whichever guidelines this is trying to follow need to be enforced automatically by a build, e.g. make lint must fail if guidelines are not observed. Without the automatic verification it's not worth making these changes.

pravarag · 2019-01-10T10:02:34Z

It seems 'flake-8' is already present as a style guiding linter in make file. But still I can see some errors in the code related to PEP-8 format. Can we enforce PEP-8 in .travis.yml so that pep-8 can check the .py files for any violations?

black-adder · 2019-01-10T15:54:28Z

looks like we already run flake-8 in travis https://github.com/jaegertracing/jaeger-client-python/blob/master/.travis.yml#L33; are we possibly swallowing the errors in travis?

yurishkuro · 2019-01-10T18:21:50Z

But still I can see some errors in the code related to PEP-8 format.

@pravarag I don't understand what you mean. There are no errors from flake8:

$ make lint
flake8 jaeger_client crossdock tests
./scripts/check-license.sh

which is why I asked above: when you say "still I can see some errors", which linter are you running?

-- UPDATE --
the reason lint above wasn't failing was because tests were excluded from flake8 config in setup.cfg, so including the directory in the command had no effect

yurishkuro · 2019-01-10T19:01:40Z

related #231

pravarag · 2019-01-11T06:08:35Z

@yurishkuro with errors in tests, I meant that code in few of the test files is still not indented as per PEP8. As you have mentioned in #231 , that tests is excluded from flake8 configuration. So, any chance tests will be included in flake8 configuration in near future?

yurishkuro · 2019-01-12T19:29:49Z

@pravarag I am now clear on what was happening (I updated my #227 (comment) above). If you would like to finish this PR and address the comments, we will merge it and enforce linting of tests going forward.

pravarag · 2019-01-14T10:26:01Z

@yurishkuro , I've updated the code with the following
-> In make file, added tests in "lint: "
-> In setup.cfg, removed entry for tests in the exclude list

now, make lint reports PEP8 errors if there are any in jaeger-client, crossdock & tests.
Please review the code.

codecov · 2019-01-15T13:28:29Z

Codecov Report

Merging #227 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #227   +/-   ##
=======================================
  Coverage   94.59%   94.59%           
=======================================
  Files          25       25           
  Lines        1867     1867           
  Branches      247      247           
=======================================
  Hits         1766     1766           
  Misses         66       66           
  Partials       35       35

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3f091d4...be99043. Read the comment docs.

bhavin192 · 2019-01-16T13:35:08Z

tests/test_TUDPTransport.py

@@ -13,10 +13,8 @@
 # limitations under the License.

 from __future__ import absolute_import
-
-import mock
+# import mock  # noqa: F401


Instead of commenting, I think we should remove the import if it's not used

bhavin192 · 2019-01-16T13:35:59Z

tests/test_TUDPTransport.py

 import unittest
-


Let's keep this blank line to separate the standard imports from out own modules

bhavin192 · 2019-01-16T13:37:38Z

tests/test_config.py

@@ -133,18 +133,19 @@ def test_throttler(self):

    def test_for_unexpected_config_entries(self):
        with self.assertRaises(Exception):
-            _ = Config({"unexpected":"value"}, validate=True)
+            _ = Config({'unexpected': 'value'}, validate=True)
+            assert _


Can you do this same as you did for other cases? i.e. just call Config({"unexpected":"value"}, validate=True)

what is the purpose of adding assert, since it's never expected to be reached?

bhavin192 · 2019-01-16T13:38:13Z

tests/test_crossdock.py

@@ -1,13 +1,13 @@
 # Copyright (c) 2016-2018 Uber Technologies, Inc.
 #
-# Licensed under the Apache License, Version 2.0 (the "License");
+# Licensed under the Apache License, Version 2.0 (the 'License');


Please revert this

please do not change license headers, they are standard and not subject to code guidelines

okay, I'll revert back the changes..

bhavin192 · 2019-01-16T13:38:23Z

tests/test_crossdock.py

 # you may not use this file except in compliance with the License.
 # You may obtain a copy of the License at
 #
 # http://www.apache.org/licenses/LICENSE-2.0
 #
 # Unless required by applicable law or agreed to in writing, software
-# distributed under the License is distributed on an "AS IS" BASIS,
+# distributed under the License is distributed on an 'AS IS' BASIS,


This as well

bhavin192 · 2019-01-16T13:49:44Z

tests/test_reporter.py

@@ -12,7 +13,7 @@
 # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 # See the License for the specific language governing permissions and
 # limitations under the License.
-
+#


This as well?

bhavin192 · 2019-01-16T13:50:10Z

tests/test_sampler.py

@@ -1,18 +1,19 @@
 from __future__ import division
 # Copyright (c) 2016-2018 Uber Technologies, Inc.
 #
-# Licensed under the Apache License, Version 2.0 (the "License");
+# Licensed under the Apache License, Version 2.0 (the 'License');


Same as above

bhavin192 · 2019-01-16T13:50:19Z

tests/test_sampler.py

 # you may not use this file except in compliance with the License.
 # You may obtain a copy of the License at
 #
 # http://www.apache.org/licenses/LICENSE-2.0
 #
 # Unless required by applicable law or agreed to in writing, software
-# distributed under the License is distributed on an "AS IS" BASIS,
+# distributed under the License is distributed on an 'AS IS' BASIS,


Same as above

bhavin192 · 2019-01-16T13:51:52Z

tests/test_tracer.py

@@ -1,13 +1,13 @@
 # Copyright (c) 2016 Uber Technologies, Inc.
 #
-# Licensed under the Apache License, Version 2.0 (the "License");
+# Licensed under the Apache License, Version 2.0 (the 'License');


Same as above

bhavin192 · 2019-01-16T13:51:59Z

tests/test_tracer.py

 # you may not use this file except in compliance with the License.
 # You may obtain a copy of the License at
 #
 # http://www.apache.org/licenses/LICENSE-2.0
 #
 # Unless required by applicable law or agreed to in writing, software
-# distributed under the License is distributed on an "AS IS" BASIS,
+# distributed under the License is distributed on an 'AS IS' BASIS,


Same as above

yurishkuro · 2019-01-17T04:45:15Z

tests/test_reporter.py

@@ -1,4 +1,5 @@
 from __future__ import print_function


the import should go after standard license header

Okay, I'll change this as well..

yurishkuro

lgtm, some small corrections, and please do not change the license headers

Changes for including flake8 in tests folder Signed-off-by: pravar <pravarag@gmail.com>

pravarag · 2019-01-18T05:15:11Z

@yurishkuro, I've done the corrections and reverted back the changes to license headers. Please review.

yurishkuro · 2019-01-18T05:43:41Z

tests/test_utils.py:80:1: E302 expected 2 blank lines, found 1
tests/test_utils.py:82:49: W292 no newline at end of file

…thon

pravarag · 2019-01-18T06:24:28Z

It seems these below lines were recently added in test_utils.py

def test_get_local_ip_by_socket_does_not_blow_up():
    import jaeger_client.utils
    jaeger_client.utils.get_local_ip_by_socket()

That's why the build failed
Pulled the latest changes and committed the code.

yurishkuro · 2019-01-18T15:24:58Z

Nicely done!

pravarag · 2019-01-19T04:57:27Z

Thanks!! will continue contributing :-)

yurishkuro · 2019-01-19T18:29:56Z

Much appreciated (#235 - wink, wink).

pravarag changed the title ~~Tests formatted as per PEP8 instructions~~ Tests formatted as per PEP8 guidelines Dec 20, 2018

yurishkuro reviewed Dec 28, 2018

View reviewed changes

pravarag force-pushed the master branch from c1e303b to ac66b26 Compare January 15, 2019 13:22

pravarag force-pushed the master branch 3 times, most recently from 4d4b8f0 to a69311f Compare January 16, 2019 04:39

bhavin192 suggested changes Jan 16, 2019

View reviewed changes

yurishkuro reviewed Jan 17, 2019

View reviewed changes

pravarag force-pushed the master branch from a69311f to 14bddcc Compare January 17, 2019 05:32

Tests formatted as per PEP8 instructions

bf7cbbd

Changes for including flake8 in tests folder Signed-off-by: pravar <pravarag@gmail.com>

pravarag force-pushed the master branch from 14bddcc to bf7cbbd Compare January 17, 2019 07:15

Merge branch 'master' into master

748da26

ghost assigned yurishkuro Jan 18, 2019

ghost added the review label Jan 18, 2019

yurishkuro approved these changes Jan 18, 2019

View reviewed changes

Merge branch 'master' into master

4c476fc

Merge branch 'master' of https://github.com/pravarag/jaeger-client-py…

be99043

…thon

yurishkuro merged commit 0ce73e3 into jaegertracing:master Jan 18, 2019

ghost removed the review label Jan 18, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tests formatted as per PEP8 guidelines #227

Tests formatted as per PEP8 guidelines #227

pravarag commented Dec 20, 2018 •

edited

Loading

black-adder commented Dec 20, 2018

pravarag commented Dec 21, 2018

yurishkuro Dec 28, 2018

yurishkuro commented Dec 29, 2018

pravarag commented Jan 10, 2019

black-adder commented Jan 10, 2019

yurishkuro commented Jan 10, 2019 •

edited

Loading

yurishkuro commented Jan 10, 2019

pravarag commented Jan 11, 2019

yurishkuro commented Jan 12, 2019

pravarag commented Jan 14, 2019 •

edited

Loading

codecov bot commented Jan 15, 2019 •

edited

Loading

bhavin192 Jan 16, 2019

bhavin192 Jan 16, 2019

bhavin192 Jan 16, 2019

yurishkuro Jan 17, 2019

bhavin192 Jan 16, 2019

yurishkuro Jan 17, 2019

pravarag Jan 17, 2019

bhavin192 Jan 16, 2019

bhavin192 Jan 16, 2019

bhavin192 Jan 16, 2019

bhavin192 Jan 16, 2019

bhavin192 Jan 16, 2019

bhavin192 Jan 16, 2019

yurishkuro Jan 17, 2019

pravarag Jan 17, 2019

yurishkuro left a comment

pravarag commented Jan 18, 2019

yurishkuro commented Jan 18, 2019

pravarag commented Jan 18, 2019

yurishkuro commented Jan 18, 2019

pravarag commented Jan 19, 2019

yurishkuro commented Jan 19, 2019

Tests formatted as per PEP8 guidelines #227

Tests formatted as per PEP8 guidelines #227

Conversation

pravarag commented Dec 20, 2018 • edited Loading

black-adder commented Dec 20, 2018

pravarag commented Dec 21, 2018

Choose a reason for hiding this comment

yurishkuro commented Dec 29, 2018

pravarag commented Jan 10, 2019

black-adder commented Jan 10, 2019

yurishkuro commented Jan 10, 2019 • edited Loading

yurishkuro commented Jan 10, 2019

pravarag commented Jan 11, 2019

yurishkuro commented Jan 12, 2019

pravarag commented Jan 14, 2019 • edited Loading

codecov bot commented Jan 15, 2019 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurishkuro left a comment

Choose a reason for hiding this comment

pravarag commented Jan 18, 2019

yurishkuro commented Jan 18, 2019

pravarag commented Jan 18, 2019

yurishkuro commented Jan 18, 2019

pravarag commented Jan 19, 2019

yurishkuro commented Jan 19, 2019

pravarag commented Dec 20, 2018 •

edited

Loading

yurishkuro commented Jan 10, 2019 •

edited

Loading

pravarag commented Jan 14, 2019 •

edited

Loading

codecov bot commented Jan 15, 2019 •

edited

Loading