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

added github workflow for unit tests #328

Merged
merged 12 commits into from
Feb 9, 2021

Conversation

willarmiros
Copy link
Contributor

Which problem is this PR solving?

Short description of the changes

Mostly just a copy of #272, with added paths for caching. I'm leaving CircleCI config in for now to allow both tests to run in parallel, but will remove them once I confirm everything passes on GH Actions. From there I believe it's up to the maintainers to remove CircleCI checks from branch protection & disable the workflow on CircleCI.

Next steps are to migrate container images test images from CircleCI to GHCR, then make a CD workflow using GH Actions which will require more discussion.

@willarmiros willarmiros requested a review from a team January 28, 2021 22:24
@codecov
Copy link

codecov bot commented Jan 28, 2021

Codecov Report

Merging #328 (07c7a20) into main (17d9445) will decrease coverage by 1.13%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #328      +/-   ##
==========================================
- Coverage   95.02%   93.88%   -1.14%     
==========================================
  Files         135       12     -123     
  Lines        7194      409    -6785     
  Branches      672       45     -627     
==========================================
- Hits         6836      384    -6452     
+ Misses        358       25     -333     
Impacted Files Coverage Δ
.../opentelemetry-plugin-mongodb/test/mongodb.test.ts
.../opentelemetry-instrumentation-graphql/src/enum.ts
...ins/node/opentelemetry-plugin-pg-pool/.eslintrc.js
...ry-instrumentation-user-interaction/src/version.ts
...pentelemetry-resource-detector-github/.eslintrc.js
...entelemetry-instrumentation-graphql/src/symbols.ts
...metry-instrumentation-graphql/test/graphql.test.ts
node/opentelemetry-plugin-ioredis/src/ioredis.ts
.../opentelemetry-plugin-dns/test/utils/assertSpan.ts
node/opentelemetry-plugin-mysql/src/utils.ts
... and 113 more

@willarmiros
Copy link
Contributor Author

Fixed coverage to only run once (on Node 12) and removed CircleCI (which caused the CI failure). This should be ready for review!

Copy link
Member

@johnbley johnbley left a comment

Choose a reason for hiding this comment

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

Looks like a good port to me, nice work.

@willarmiros
Copy link
Contributor Author

Thanks @johnbley! I have no idea why the codecov is dropping on this, I thought running coverage on all Node versions instead of just Node 12 might help, but I guess not. Any ideas/does this impact merging?

@johnbley
Copy link
Member

Thanks @johnbley! I have no idea why the codecov is dropping on this, I thought running coverage on all Node versions instead of just Node 12 might help, but I guess not. Any ideas/does this impact merging?

It doesn't look like a real problem to me, possibly just a blip owing to something not lining up right. The root checkout directories changed from/root/project/detectors/node/ to /__w/ and that might be confusing the codecov report? In any case it doesn't show any specific instances of actual decreased coverage, so I'm not particularly worried.

- name: Unit tests
run: npm run test
- name: Report Coverage
run: npm run codecov
Copy link
Member

Choose a reason for hiding this comment

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

Looks like this will report coverage for each version of node, not just 14

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yeah I removed the conditional here to see if it would fix the codecov diff, I'll add it back in.

Copy link
Member

Choose a reason for hiding this comment

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

I'm going to approve with the assumption you will fix this before it gets merged

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should be good now :) thanks for the catch

@dyladan dyladan merged commit dbbf02f into open-telemetry:main Feb 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants