Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add test coverage collecting #128

Merged
merged 5 commits into from
Nov 7, 2019
Merged

Add test coverage collecting #128

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c9ddd65
Add test coverage collecting
Jamim Oct 30, 2019
c012914
Revert changes to generated jaeger modules
c24t Nov 5, 2019
cdc64f5
Only report coverage for package being tested
c24t Nov 5, 2019
3da192d
Ignore generated files in coverage
c24t Nov 5, 2019
89c4f01
Remove pytest-cov.ini
Jamim Nov 6, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .coveragerc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
[run]
omit =
*/tests/*
*/setup.py
*/gen/*
1 change: 1 addition & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ venv*/
pip-log.txt

# Unit test / coverage reports
coverage.xml
.coverage
.nox
.tox
Expand Down
4 changes: 4 additions & 0 deletions .travis.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,3 +19,7 @@ install:

script:
- tox

after_success:
- pip install codecov
- codecov -v
52 changes: 27 additions & 25 deletions opentelemetry-api/src/opentelemetry/context/async_context.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,32 +12,34 @@
# See the License for the specific language governing permissions and
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this file was changed by mistake? Could you please remove these changes from the PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not, it wasn't.
This file was changed in purpose to make it "importable" for Python <3.7.
Please see a corresponding build. That build failed with:

==================================== ERRORS ====================================
_ ERROR collecting opentelemetry-api/src/opentelemetry/context/async_context.py _
ImportError while importing test module '/home/travis/build/Jamim/opentelemetry-python/opentelemetry-api/src/opentelemetry/context/async_context.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
opentelemetry-api/src/opentelemetry/context/async_context.py:16: in <module>
    from contextvars import ContextVar
E   ModuleNotFoundError: No module named 'contextvars'

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But doesn't this break the logic of the importing __init__.py? You are not supposed to import that file directly anyway.

Copy link
Contributor Author

@Jamim Jamim Oct 24, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But doesn't this break the logic of the importing __init__.py?

No, it doesn't.

Python 3.7 
$ python
Python 3.7.3 (default, Apr  3 2019, 19:16:38) 
[GCC 8.0.1 20180414 (experimental) [trunk revision 259383]] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> from opentelemetry.context import Context
>>> Context
AsyncRuntimeContext({})
Python 3.6 
$ python
Python 3.6.8 (default, Oct  7 2019, 12:59:55) 
[GCC 8.3.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> from opentelemetry.context import Context
>>> Context
ThreadLocalRuntimeContext({})

You are not supposed to import that file directly anyway.

Having non-importable files in the project is not good in general.

# limitations under the License.

import typing # pylint: disable=unused-import
from contextvars import ContextVar
try:
from contextvars import ContextVar
except ImportError:
pass
else:
c24t marked this conversation as resolved.
Show resolved Hide resolved
import typing # pylint: disable=unused-import
from . import base_context

from . import base_context
class AsyncRuntimeContext(base_context.BaseRuntimeContext):
class Slot(base_context.BaseRuntimeContext.Slot):
def __init__(self, name: str, default: object):
# pylint: disable=super-init-not-called
self.name = name
self.contextvar = ContextVar(name) # type: ContextVar[object]
self.default = base_context.wrap_callable(
default
) # type: typing.Callable[..., object]

def clear(self) -> None:
self.contextvar.set(self.default())

class AsyncRuntimeContext(base_context.BaseRuntimeContext):
class Slot(base_context.BaseRuntimeContext.Slot):
def __init__(self, name: str, default: "object"):
# pylint: disable=super-init-not-called
self.name = name
self.contextvar = ContextVar(name) # type: ContextVar[object]
self.default = base_context.wrap_callable(
default
) # type: typing.Callable[..., object]
def get(self) -> object:
try:
return self.contextvar.get()
except LookupError:
value = self.default()
self.set(value)
return value

def clear(self) -> None:
self.contextvar.set(self.default())

def get(self) -> "object":
try:
return self.contextvar.get()
except LookupError:
value = self.default()
self.set(value)
return value

def set(self, value: "object") -> None:
self.contextvar.set(value)
def set(self, value: object) -> None:
self.contextvar.set(value)
2 changes: 2 additions & 0 deletions pytest-cov.ini
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
[pytest]
addopts = -rs -v
2 changes: 2 additions & 0 deletions pytest.ini
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
[pytest]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know @c24t had some reservations about using pytest. We were supposed to discuss this after the alpha release.

FWIW I really like pytest and wholeheartedly support using pytest and pytest functionality.

Copy link
Contributor Author

@Jamim Jamim Sep 10, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know @c24t had some reservations about using pytest.

Hello @c24t, can you please clarify what kind of reservations you had?
Thank you!

We were supposed to discuss this after the alpha release.

Hi @toumorokoshi,
I think nothing prevents us from discussing it a bit earlier.

Also, I found a related issue: #28.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My biggest complaints about pytest have to do with fixtures. Fixtures with side effects, autouse fixtures, and the monkeypatch fixture in particular have the potential to turn any test suite into a horrifying mess. Using fixtures as arguments in tests makes working with tests difficult outside of pytest, and adds a lot of magic. I don't mean to say it's not possible to write nice tests with pytest, just that the library encourages some pretty bad (IMHO) behavior.

When we considered adding it before it wasn't clear that there were any features of pytest that we couldn't get just as easily from the built in unittest library. It may be that the extra complexity is justified if pytest makes writing and running tests easier. I still don't see a strong reason for using pytest in this PR, but I may be missing something here. What makes pytest --cov better than coverage run?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @toumorokoshi and @c24t,

On the SIG meeting 2019-09-12 we discussed migration to pytest and, if I recall correctly, we found that most developers are fine with it or have no opinion. Unfortunately, we didn't write out a corresponding decision in the OT Python SIG Notes.
So my question is, can we resolve this thread now and move forward with pytest or is it necessary to have an additional discussion?

Thank you!

Copy link
Member

@toumorokoshi toumorokoshi Oct 10, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I've pinged the appropriate opinionated people regarding the use of pytest, so I'm going to remove my "changes required". I'll let others who are against pytest argue their point.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@toumorokoshi To actually dismiss your review you have to expand the "Changes requested" box at the bottom of the PR, and then click "..." -> "Dismiss" next to your review there. Re-requesting your review won't actually dismiss it (I ran into that too once, it's not the most intuitive UI).

addopts = -rs -v
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

duplicate addopts?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean that there is the same line inpytest-cov.ini?
That file is a separate config file used in the coverage testenv only.
It's required in order to add python_files = *.py setting which, I assume, should not be used in the test testenv.

28 changes: 28 additions & 0 deletions scripts/coverage.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
#!/bin/bash

set -e

function cov {
pytest \
-c pytest-cov.ini \
--ignore-glob=*/setup.py \
--cov ${1} \
--cov-append \
--cov-branch \
--cov-report='' \
${1}
}


coverage erase

cov opentelemetry-api
cov opentelemetry-sdk
cov ext/opentelemetry-ext-http-requests
cov ext/opentelemetry-ext-jaeger
cov ext/opentelemetry-ext-opentracing-shim
cov ext/opentelemetry-ext-wsgi
cov examples/opentelemetry-example-app

coverage report
coverage xml
21 changes: 20 additions & 1 deletion tox.ini
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,11 @@ skip_missing_interpreters = True
envlist =
py3{4,5,6,7,8}-test-{api,sdk,example-app,ext-wsgi,ext-http-requests,ext-jaeger,opentracing-shim}
pypy3-test-{api,sdk,example-app,ext-wsgi,ext-http-requests,ext-jaeger,opentracing-shim}
py3{4,5,6,7,8}-coverage
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will this double execute all tests? Since under the hood it's calling pytest --cov, which IIUC will run all the test again.

If it is, I think we should eliminate duplicate tests before merging.

Copy link
Contributor Author

@Jamim Jamim Oct 11, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will this double execute all tests?

Yes, it will.

If it is, I think we should eliminate duplicate tests before merging.

I don't think so currently.

There are reasons why:

cc @Oberon00, @c24t

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I worry that this will double the build times in CI and locally for those running tox.

I don't want to block this PR if this is the direction people want to go, but I think we should go back and find a way to not double-execute tests. Can you file a ticket once this is merged so we remember we should go back and hopefully resolve this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I'll submit a corresponding issue once this PR is merged.


; Coverage is temporarily disabled for pypy3 due to the pytest bug.
; pypy3-coverage

lint
py37-tracecontext
py37-{mypy,mypyinstalled}
Expand All @@ -15,6 +20,8 @@ python =

[testenv]
deps =
test: pytest
coverage: pytest-cov
mypy,mypyinstalled: mypy~=0.740

setenv =
Expand Down Expand Up @@ -45,12 +52,24 @@ commands_pre =
jaeger: pip install {toxinidir}/ext/opentelemetry-ext-jaeger
opentracing-shim: pip install {toxinidir}/opentelemetry-sdk {toxinidir}/ext/opentelemetry-ext-opentracing-shim

; In order to get a healthy coverage report,
; we have to install packages in editable mode.
coverage: pip install -e {toxinidir}/opentelemetry-api
coverage: pip install -e {toxinidir}/opentelemetry-sdk
coverage: pip install -e {toxinidir}/ext/opentelemetry-ext-azure-monitor
coverage: pip install -e {toxinidir}/ext/opentelemetry-ext-http-requests
coverage: pip install -e {toxinidir}/ext/opentelemetry-ext-jaeger
coverage: pip install -e {toxinidir}/ext/opentelemetry-ext-opentracing-shim
coverage: pip install -e {toxinidir}/ext/opentelemetry-ext-wsgi
coverage: pip install -e {toxinidir}/examples/opentelemetry-example-app

; Using file:// here because otherwise tox invokes just "pip install
; opentelemetry-api", leading to an error
mypyinstalled: pip install file://{toxinidir}/opentelemetry-api/

commands =
test: python -m unittest discover
test: pytest
coverage: {toxinidir}/scripts/coverage.sh

mypy: mypy --namespace-packages opentelemetry-api/src/opentelemetry/
; For test code, we don't want to enforce the full mypy strictness
Expand Down