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

[extensions] Better document. Add unit tests. (#1248) #1442

Merged
merged 1 commit into from Jan 4, 2020

Conversation

@ivo-stefchev
Copy link
Contributor

ivo-stefchev commented Aug 5, 2019

I was curious of how this rule worked, so I went the easiest way and wrote a bunch of unit tests for the buildProperties (a method that returns the options from the config for the extensions rule) method.

Closes #1248.
@coveralls

This comment has been minimized.

Copy link

coveralls commented Aug 5, 2019

Coverage Status

Coverage decreased (-0.02%) to 96.26% when pulling b511da2 on ivo-stefchev:#1248 into fb0cbeb on benmosher:master.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Aug 5, 2019

Coverage Status

Coverage increased (+2.6%) to 97.897% when pulling bd733c3 on ivo-stefchev:#1248 into 35a12f9 on benmosher:master.


const ruleTester = new RuleTester()

describe('extensions buildProperties', function() {

This comment has been minimized.

Copy link
@ljharb

ljharb Aug 19, 2019

Collaborator

i'm confused what the purpose of these tests are. can the tests be exercised with ruleTester test cases, rather than hardcoding to things like __test__?

This comment has been minimized.

Copy link
@ivo-stefchev

ivo-stefchev Sep 3, 2019

Author Contributor

Sorry, I deleted my previous comment by accident. I did it like that, because buildProperties is private function and needs to be tested.
So I guess it's a no go for this PR?

This comment has been minimized.

Copy link
@ljharb

ljharb Sep 4, 2019

Collaborator

in general, private things shouldn't be tested - in this plugin's case, everything should be tested via the RuleTester.

This comment has been minimized.

Copy link
@soryy708

soryy708 Oct 23, 2019

I'm not sure why you're saying "private things shouldn't be tested". I see no reason not to, and plenty of reasons to do.

This comment has been minimized.

Copy link
@golopot

golopot Oct 23, 2019

Contributor

One benefit of not testing internal functions is that you can refactor internals as much as you want.

This comment has been minimized.

Copy link
@soryy708

soryy708 Oct 23, 2019

But when you refactor internals and break something, you wish you had tests to catch that early.

This comment has been minimized.

Copy link
@ljharb

ljharb Oct 23, 2019

Collaborator

The tests should be on the public pieces, and should transparently exercise all the private pieces.

Any behavior of your private code that is not observable via your public code should not be tested because it doesn't matter if it works or not

@ljharb ljharb force-pushed the ivo-stefchev:#1248 branch from bd733c3 to b511da2 Jan 4, 2020
@ljharb
ljharb approved these changes Jan 4, 2020
Copy link
Collaborator

ljharb left a comment

I've gone ahead and reverted the semver-minor piece that exposes a utility function, and the associated tests, so I can merge the docs improvement.

@ljharb ljharb merged commit b511da2 into benmosher:master Jan 4, 2020
1 of 3 checks passed
1 of 3 checks passed
continuous-integration/appveyor/pr AppVeyor build failed
Details
coverage/coveralls Coverage decreased (-0.02%) to 96.26%
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ljharb ljharb added the docs label Jan 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.