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

some tests around the NpmList.maybePushNewCoordinate() logic. #188

Open
wants to merge 4 commits into
base: PlayingWithRequiredBy
Choose a base branch
from

Conversation

bhamail
Copy link
Contributor

@bhamail bhamail commented Mar 24, 2020

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

@bhamail bhamail requested a review from DarthHater March 24, 2020 00:56
});
});

describe('NpmList.maybePushNewCoordinate', async () => {
Copy link
Member

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?

Copy link
Contributor Author

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(' Path: supPath\n')).to.not.eq(-1);
Copy link
Member

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?

Copy link
Contributor Author

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', () => {
Copy link
Member

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.

Copy link
Contributor Author

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(' Required By: \n')).to.not.eq(-1);
});

it('should include supplemental', () => {
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants