-
Notifications
You must be signed in to change notification settings - Fork 624
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Unit tests #42
Unit tests #42
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
# Copyright 2019, OpenTelemetry Authors | ||
# | ||
# Licensed under the Apache License, Version 2.0 (the "License"); | ||
# 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, | ||
# 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. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,95 @@ | ||
# Copyright 2019, OpenTelemetry Authors | ||
# | ||
# Licensed under the Apache License, Version 2.0 (the "License"); | ||
# 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, | ||
# 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. | ||
|
||
from importlib import reload | ||
import sys | ||
import unittest | ||
import os | ||
|
||
from opentelemetry import loader | ||
from opentelemetry import trace | ||
|
||
DUMMY_TRACER = None | ||
|
||
class DummyTracer(trace.Tracer): | ||
pass | ||
|
||
def get_opentelemetry_implementation(type_): | ||
global DUMMY_TRACER #pylint:disable=global-statement | ||
assert type_ is trace.Tracer | ||
DUMMY_TRACER = DummyTracer() | ||
return DUMMY_TRACER | ||
|
||
#pylint:disable=redefined-outer-name,protected-access,unidiomatic-typecheck | ||
|
||
class TestLoader(unittest.TestCase): | ||
|
||
def setUp(self): | ||
reload(loader) | ||
reload(trace) | ||
|
||
# Need to reload self, otherwise DummyTracer will have the wrong base | ||
# class after reloading `trace`. | ||
reload(sys.modules[__name__]) | ||
|
||
|
||
def test_get_default(self): | ||
tracer = trace.tracer() | ||
self.assertIs(type(tracer), trace.Tracer) | ||
|
||
def test_preferred_impl(self): | ||
trace.set_preferred_tracer_implementation( | ||
get_opentelemetry_implementation) | ||
tracer = trace.tracer() | ||
self.assertIs(tracer, DUMMY_TRACER) | ||
|
||
# NOTE: We use do_* + *_<arg> methods because subtest wouldn't run setUp, | ||
# which we require here. | ||
def do_test_preferred_impl(self, setter): | ||
setter(get_opentelemetry_implementation) | ||
tracer = trace.tracer() | ||
self.assertIs(tracer, DUMMY_TRACER) | ||
def test_preferred_impl_with_tracer(self): | ||
self.do_test_preferred_impl(trace.set_preferred_tracer_implementation) | ||
def test_preferred_impl_with_default(self): | ||
self.do_test_preferred_impl( | ||
loader.set_preferred_default_implementation) | ||
|
||
def test_try_set_again(self): | ||
self.assertTrue(trace.tracer()) | ||
# Try setting after the tracer has already been created: | ||
with self.assertRaises(RuntimeError) as einfo: | ||
trace.set_preferred_tracer_implementation( | ||
get_opentelemetry_implementation) | ||
self.assertIn('already loaded', str(einfo.exception)) | ||
|
||
def do_test_get_envvar(self, envvar_suffix): | ||
global DUMMY_TRACER #pylint:disable=global-statement | ||
|
||
# Test is not runnable with this! | ||
self.assertFalse(sys.flags.ignore_environment) | ||
|
||
envname = 'OPENTELEMETRY_PYTHON_IMPLEMENTATION_' + envvar_suffix | ||
os.environ[envname] = __name__ | ||
try: | ||
tracer = trace.tracer() | ||
self.assertIs(tracer, DUMMY_TRACER) | ||
finally: | ||
DUMMY_TRACER = None | ||
del os.environ[envname] | ||
self.assertIs(type(tracer), DummyTracer) | ||
def test_get_envvar_tracer(self): | ||
return self.do_test_get_envvar('TRACER') | ||
def test_get_envvar_default(self): | ||
return self.do_test_get_envvar('DEFAULT') |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -1,6 +1,6 @@ | ||||||
[tox] | ||||||
skipsdist = True | ||||||
envlist = py37-{lint,mypy}, docs | ||||||
envlist = py37-{lint,mypy,test}, docs | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How about running the tests in all supported python versions (#25 notwithstanding)?
Suggested change
Also note that this turns up a type annotation in the loader as unsupported in 3.4. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's definitely something we should do but when I first turned that on I run into "mypy does not support Python 3.4 and decided to do that later and reopen #25 first. I'll take another stab at fixing the 3.4 issues. |
||||||
|
||||||
[travis] | ||||||
python = | ||||||
|
@@ -11,10 +11,23 @@ deps = | |||||
lint: pylint~=2.3.1 | ||||||
mypy: mypy~=0.711 | ||||||
|
||||||
setenv = | ||||||
PYTHONPATH={toxinidir}/opentelemetry-api/src/ | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you're doing this to get There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe it would. Why do we use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IIRC because we've got multiple packages under separate directories so tox didn't know what to install. |
||||||
mypy: MYPYPATH={env:PYTHONPATH} | ||||||
|
||||||
changedir = | ||||||
test: opentelemetry-api/tests | ||||||
|
||||||
|
||||||
commands = | ||||||
py37-lint: pylint opentelemetry-api/src/opentelemetry/ | ||||||
; Prefer putting everything in one pylint command to profit from duplication | ||||||
; warnings. | ||||||
py37-lint: pylint opentelemetry-api/src/opentelemetry/ opentelemetry-api/tests/ | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we should have the unit tests run before the lint/mypy. When developing, I usually fix the lint issues at the end once all the code has been completed and finalized. If the build were to fail on the tests with perfect lint, after the tests are fixed, the lint might be broken due to the changes. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it's better in general to fail fast. If our main use case here is CI I'd like to see errors as soon as possible. In any case the ordering doesn't matter if we run the tests in parallel (with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. True, the order would be defined with the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I also think unit tests should run before the lint/mypy step ;) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Me too, personally I think the ordering in OpenCensus makes sense.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I sometimes got better error messages from pylint than from my failed unit test about e.g. some non-existent variable. On the other hand I agree that spending time on making the line-breaks nice before the unit test runs through is futile. So personally I think my ordering would be:
In any case, like @c24t said, the ordering does not matter that much, so I'm really fine with anything. |
||||||
py37-mypy: mypy opentelemetry-api/src/opentelemetry/ | ||||||
; For test code, we don't want to use the full mypy strictness, so we use its | ||||||
; default flags instead. | ||||||
py37-mypy: mypy opentelemetry-api/tests/ --config-file= | ||||||
test: python -m unittest discover | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: python -m unittest is equivalent to python -m unittest discover There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I saw that in the unittest docs, but went with "explicit is better than implicit" |
||||||
|
||||||
[testenv:docs] | ||||||
deps = | ||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like
pylint
doesn't care about blank lines... do you think it'd be overkill to runflake8
with lint too?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For your consideration: #46. 😄