-
Notifications
You must be signed in to change notification settings - Fork 53
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
some tests around the NpmList.maybePushNewCoordinate() logic. #188
base: PlayingWithRequiredBy
Are you sure you want to change the base?
Conversation
}); | ||
}); | ||
|
||
describe('NpmList.maybePushNewCoordinate', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have a gigantic problem with this, but this is a private method. Generally I'd want to test the publics, and not tie directly to privates (brittle situation). Maybe we need to retinker this class a bit to make it easier to test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I felt a little dirty calling "private" directly, but less dirty than trying to test the big ole outer method. Not sure I see a clear path to retinker. I was just happy to manage to cover all the cases. ;)
); | ||
|
||
expect(result).to.equal(false); | ||
expect(written.indexOf(' [32mPath: supPath[39m\n')).to.not.eq(-1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Holy guac, what is the [32m
' stuff?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume it is color stuff
@@ -75,4 +94,36 @@ describe('AuditOSSIndex', () => { | |||
|
|||
expect(result).to.equal(false); | |||
}); | |||
|
|||
it('should include supplemental when empty', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both of these tests fail for me locally, currently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you share the failure info?
expect(written.indexOf(' [32mRequired By: [39m\n')).to.not.eq(-1); | ||
}); | ||
|
||
it('should include supplemental', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both of these tests fail for me locally, currently.
This is what you get when a noob tries to add some unit tests.
If not too objectionable, these could be merged to the original PlayingWithRequiredBy branch.
Suggestions welcome.
cc @bhamail / @DarthHater / @allenhsieh / @ken-duck