Skip to content
This repository has been archived by the owner on Oct 3, 2023. It is now read-only.

Use ts-mocha to run tests #444

Merged
merged 14 commits into from
Mar 27, 2019
Merged

Use ts-mocha to run tests #444

merged 14 commits into from
Mar 27, 2019

Conversation

kenashcraft
Copy link
Contributor

@kenashcraft kenashcraft commented Mar 25, 2019

This gives good stack traces during tests and means we can skip the compile step. Converts all packages except:

  • opencensus-nodejs: has tests that expect .js files/syntax
  • opencensus-instrumentation-redis: I couldn't figure out what was going wrong
  • opencensus-exporter-ocagent: Depends on the compilation step to copy files around

@mayurkale22
Copy link
Member

@kenashcraft thanks for the contribution. Please rebase the PR, this should fix the build. We have already removed the dependencies on @types/pkg-dir and pkg-dir #438.

@codecov-io
Copy link

codecov-io commented Mar 25, 2019

Codecov Report

Merging #444 into master will decrease coverage by 0.16%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #444      +/-   ##
==========================================
- Coverage      95%   94.83%   -0.17%     
==========================================
  Files         147      146       -1     
  Lines        9566     9739     +173     
  Branches      680      692      +12     
==========================================
+ Hits         9088     9236     +148     
- Misses        478      503      +25
Impacted Files Coverage Δ
test/test-instana.ts 88.46% <0%> (-11.54%) ⬇️
src/detect-resource.ts 92.85% <0%> (-7.15%) ⬇️
src/common-utils.ts 95.83% <0%> (-4.17%) ⬇️
src/tracecontext-format.ts 96.42% <0%> (-1.4%) ⬇️
...rontend/page-handlers/traceconfigz.page-handler.ts 69.56% <0%> (-1.27%) ⬇️
test/test-jaeger-format.ts 95.38% <0%> (-1.23%) ⬇️
src/tags/propagation/binary-serializer.ts 95.23% <0%> (-1.13%) ⬇️
...ages-frontend/page-handlers/tracez.page-handler.ts 60.81% <0%> (-0.53%) ⬇️
test/test-detect-resource.ts 99.51% <0%> (-0.49%) ⬇️
src/index.ts 100% <0%> (ø) ⬆️
... and 52 more

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 ac0ef77...09398ed. Read the comment docs.

"codecov": "nyc report --reporter=json && codecov -f coverage/*.json",
"clean": "rimraf build/*",
"check": "gts check",
"compile": "tsc -p .",
"compile:release": "tsc -p tsconfig-release.json",
"fix": "gts fix",
"prepare": "npm run compile:release",
"pretest": "npm run compile",
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 we should still keep this and add into unit test workflow. Something like below in .circleci/config.yml WDYT?

   - run:
        name: Compile code
        command: npm run compile

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like that config is for the entire repo. That would cause all of the other packages to run the compile step twice. Is that ok and/or should I change all of the other packages to use ts-mocha?

Copy link
Member

Choose a reason for hiding this comment

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

I would prefer to change all packages to use ts-mocha. Also, does it make sense to remove dependencies on mocha and @types/mocha after this? @draffensperger any thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, we can't remove the dependencies. The ts-mocha page says we need them: https://www.npmjs.com/package/ts-mocha

Copy link
Member

Choose a reason for hiding this comment

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

Ok SG.

@kenashcraft
Copy link
Contributor Author

@mayurkale22 Looks like I finally got all of the tests passing. I did my best to ensure I was changing only the files that needed to be changed, but please double check before we merge.

@mayurkale22
Copy link
Member

Can you please create an issue to track remaining packages.

@kenashcraft
Copy link
Contributor Author

Added issue #447.

@@ -38,6 +37,16 @@
"LICENSE",
"README.md"
],
"nyc": {
Copy link
Member

Choose a reason for hiding this comment

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

Optional: Can we move these configuration options in .nycrc, later on we can add coverage threshold and other options. Also this will make package.json more cleaner. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The official docs didn't offer that as an option: https://istanbul.js.org/docs/tutorials/typescript/ . I don't know if it is supported.

Copy link
Member

Choose a reason for hiding this comment

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

@mayurkale22 mayurkale22 merged commit 4b7d14c into census-instrumentation:master Mar 27, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants