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 unit tests for the telemetry module #471

Merged
merged 6 commits into from
Oct 17, 2019

Conversation

raphinesse
Copy link
Contributor

Motivation and Context

The telemetry module had bad test coverage and I was about to start working on some improvements in that area. So I decided to first write some decent tests for it.

Description

It might seem odd that I also tested some aspects of our dependency insight, but in some cases it was not at all clear what exactly would be happening. So I decided to just add some tests for interesting cases.

@raphinesse raphinesse changed the title Add unit tests for telemetry module Add unit tests for the telemetry module Oct 16, 2019
Copy link
Contributor

@breautek breautek left a comment

Choose a reason for hiding this comment

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

+1 but I think a ticket should be raised for filters falsy and empty arguments [T010] fix if there is no ticket already.

Copy link
Member

@erisu erisu left a comment

Choose a reason for hiding this comment

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

👍

@erisu
Copy link
Member

erisu commented Oct 17, 2019

I just noticed that two of the AppVeyor tests failed. It seems those two tests are in the describe block that uses the new mock-stdin package. I wonder if Windows is supported though the packages README does mention anything about OS requirements.

@raphinesse
Copy link
Contributor Author

+1 but I think a ticket should be raised for filters falsy and empty arguments [T010] fix if there is no ticket already.

Actually I already have a fix for that on my machine. I just wanted to get the unit tests merged first. So I will be posting a PR for that shortly.

- cli.spec#028 superseded by telemetry.spec.T017-T019
- part of cli.spec#029 superseded by telemetry.spec.T020
- cli.spec#031-032 superseded by telemetry.spec.T022-T023
@raphinesse
Copy link
Contributor Author

I just noticed that two of the AppVeyor tests failed. It seems those two tests are in the describe block that uses the new mock-stdin package. I wonder if Windows is supported though the packages README does mention anything about OS requirements.

Haha, that was my first guess too. But it turned out that insight never shows prompts if process.stdout.isTTY is false. Which is the case on AppVeyor.

@codecov-io
Copy link

codecov-io commented Oct 17, 2019

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #471     +/-   ##
=========================================
+ Coverage   79.68%   83.59%   +3.9%     
=========================================
  Files           3        3             
  Lines         256      256             
=========================================
+ Hits          204      214     +10     
+ Misses         52       42     -10
Impacted Files Coverage Δ
src/telemetry.js 96.55% <0%> (+34.48%) ⬆️

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 3ed1d5e...5151470. Read the comment docs.

@raphinesse raphinesse merged commit e6272d8 into apache:master Oct 17, 2019
@raphinesse raphinesse deleted the telemetry.spec branch October 17, 2019 12:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants