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

feat: configure CircleCI workflows #155

Merged
merged 2 commits into from Aug 5, 2019

Conversation

mayurkale22
Copy link
Member

Closes #50

@codecov-io
Copy link

codecov-io commented Aug 2, 2019

Codecov Report

Merging #155 into master will increase coverage by 4.93%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #155      +/-   ##
==========================================
+ Coverage   93.84%   98.78%   +4.93%     
==========================================
  Files          41       41              
  Lines        3282     3282              
  Branches      385      385              
==========================================
+ Hits         3080     3242     +162     
+ Misses        202       40     -162
Impacted Files Coverage Δ
...s/opentelemetry-core/test/trace/tracestate.test.ts 100% <0%> (+14.49%) ⬆️
.../opentelemetry-node-tracer/test/NodeTracer.test.ts 100% <0%> (+14.75%) ⬆️
...oks/test/asynchooks/AsyncHooksScopeManager.test.ts 100% <0%> (+15.9%) ⬆️
...pentelemetry-basic-tracer/test/BasicTracer.test.ts 100% <0%> (+17.28%) ⬆️
...etry-scope-base/test/noop/NoopScopeManager.test.ts 100% <0%> (+22.82%) ⬆️
packages/opentelemetry-basic-tracer/src/Span.ts 88.6% <0%> (+28.48%) ⬆️

command: yarn codecov
node8:
docker:
- image: node:8
Copy link
Contributor

Choose a reason for hiding this comment

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

For the node docker images that don't have browsers installed, we will need a separate test command (or environment variable) to disable the browser tests, since they run on yarn test, see

"test": "yarn test:node && yarn test:browser",

Maybe we need yarn test:node and yarn run test:browser and segment the tests in this file by that?

Copy link
Member Author

Choose a reason for hiding this comment

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

I like the idea, made the appropriate changes in the config file. ptal

Copy link
Contributor

Choose a reason for hiding this comment

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

Sweet! Now the Node and browser tests can also run in parallel!

Copy link
Member

@bg451 bg451 left a comment

Choose a reason for hiding this comment

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

i know nothing about browser testing so going to defer on that front, but the rest looks good!

@mayurkale22 mayurkale22 force-pushed the circleci branch 4 times, most recently from 59241d7 to eabe171 Compare August 2, 2019 16:59
@mayurkale22
Copy link
Member Author

c8 is supported on Node.js >= 10.12.0, looks like we need to fallback to nyc for code coverage. #93

Any thoughts?

command: yarn codecov
node8:
docker:
- image: node:8
Copy link
Contributor

Choose a reason for hiding this comment

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

Sweet! Now the Node and browser tests can also run in parallel!

@draffensperger
Copy link
Contributor

Can we just disable coverage reporting for Node 8? I would assume the coverage would be the same as other Node versions (assuming no version-specific branching).

@rochdev
Copy link
Member

rochdev commented Aug 5, 2019

c8 is supported on Node.js >= 10.12.0, looks like we need to fallback to nyc for code coverage.

I would say there are 2 options:

  1. Switch to NYC to avoid having 2 tools. This means slower builds but would allow support for more Node versions.
  2. As mentioned above, simply not running code coverage for Node 8. If there is any code specific to Node 8, it will appear as uncovered but that should be a very small portion of the codebase.

package.json Outdated Show resolved Hide resolved
@mayurkale22 mayurkale22 force-pushed the circleci branch 2 times, most recently from 9a4dd6b to f362022 Compare August 5, 2019 16:51
@mayurkale22
Copy link
Member Author

I would say there are 2 options:

  1. Switch to NYC to avoid having 2 tools. This means slower builds but would allow support for more Node versions.
  2. As mentioned above, simply not running code coverage for Node 8. If there is any code specific to Node 8, it will appear as uncovered but that should be a very small portion of the codebase.

Tests and coverage are running with c8 tool, looks like falling back to nyc is right option for now.

@mayurkale22
Copy link
Member Author

@markwolff FYI. For now, I have changed to nyc tool to support Node 8 build.

@mayurkale22 mayurkale22 merged commit e32796a into open-telemetry:master Aug 5, 2019
@mayurkale22 mayurkale22 deleted the circleci branch August 5, 2019 17:57
dyladan pushed a commit to dyladan/opentelemetry-js that referenced this pull request Sep 9, 2022
Co-authored-by: Valentin Marchaud <contact@vmarchaud.fr>
dyladan pushed a commit to dyladan/opentelemetry-js that referenced this pull request Sep 9, 2022
Co-authored-by: Valentin Marchaud <contact@vmarchaud.fr>
dyladan pushed a commit to dyladan/opentelemetry-js that referenced this pull request Sep 9, 2022
Co-authored-by: Valentin Marchaud <contact@vmarchaud.fr>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Configure CircleCI workflows
6 participants