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.

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

Add code coverage in tests #208

Closed
wants to merge 9 commits into from

Conversation

hectorhdzg
Copy link
Member

@hectorhdzg hectorhdzg commented Oct 8, 2019

This a follow up of this PR:
#128

I tried to contact Aliaksei Urbanski(Jamim) and tried to push changes to his branch with no success, so I'm creating a new PR because of this, this is addressing main concern of adding dependency on pytest, we will be using only coverage.py now

@@ -0,0 +1,5 @@
fixes:
- "^.*/site-packages/opentelemetry/sdk/::opentelemetry-sdk/src/opentelemetry/sdk/"
Copy link
Member Author

Choose a reason for hiding this comment

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

We are still testing against site-packages, apparently we want to test the package instead of the local files according to this PR #122

Copy link
Contributor

Choose a reason for hiding this comment

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

@Oberon00 What was the reasoning behind removing the "-e" again? Don't we want to run unit tests against our locally changed files?

Copy link
Member

Choose a reason for hiding this comment

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

@lzchen The -e flag was hiding issues with files that were excluded from packaging. Files were missing from the tarball / wheel files but since -e was being used, the tests were using the unpackaged files.

Copy link
Member

@Oberon00 Oberon00 Oct 9, 2019

Choose a reason for hiding this comment

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

It would be nice to have some configurable tox environment like tox -e py37-test --installation-mode=wheel or tox -e px37-test --with-coverage. But I don't think that is possible out of the box. Maybe we should generate tox.ini with some Jinja2 template 😆

@codecov-io
Copy link

codecov-io commented Oct 8, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@9fa92f3). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #208   +/-   ##
=========================================
  Coverage          ?   89.75%           
=========================================
  Files             ?       33           
  Lines             ?     1201           
  Branches          ?        0           
=========================================
  Hits              ?     1078           
  Misses            ?      123           
  Partials          ?        0

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 9fa92f3...77d3649. Read the comment docs.

@Jamim
Copy link
Contributor

Jamim commented Oct 9, 2019

Hello @hectorhdzg,

I tried to contact Aliaksei Urbanski(Jamim)

I found your message in Gitter only because I saw this PR.
I missed it due to Gitter merged a notification about that direct message to the bottom of a notification about some unrelated conversation 😕

If you really wanted to contact me, why didn't you just submit a comment on #128 and even didn't mention me using @ here? 🤔

@hectorhdzg
Copy link
Member Author

Closing in favor of @Jamim PR #128, now that he is back to business, sorry about that :)

@hectorhdzg hectorhdzg closed this Oct 9, 2019
srikanthccv pushed a commit to srikanthccv/opentelemetry-python that referenced this pull request Nov 1, 2020
* feat: add b3 format implementation

* fix: add tests

* fix: review comments and add more tests

* fix: omit SAMPLED header if sampling decision is absent
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants