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

getConfig will assign default options to generated config #429

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

PalleZingmark
Copy link
Contributor

Fix for #409

Copy link
Owner

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

Needs a test.

(thanks for sending a PR!)

src/core/done.js Outdated
@@ -24,7 +24,7 @@ var done = function() {
var msg = ''
var groupedByFile = {}
var msgGrouped
var group = this.config.groupOutputByFile
var group = 'groupOutputByFile' in this.config ? this.config.groupOutputByFile : true
Copy link
Owner

Choose a reason for hiding this comment

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

var group = this.config.groupOutputByFile != null ? this.config.groupOutputByFile : true

@PalleZingmark
Copy link
Contributor Author

Looking into writing tests for this, I actually get some weird output (on master) which to me looks a bit unintended within the test. Is this a bug or intended behaviour?
stylint-test-output

@PalleZingmark
Copy link
Contributor Author

I need to update tests.

@SimenB
Copy link
Owner

SimenB commented Oct 29, 2017

@PalleZingmark not so much intended as known 🙂 Fixed in v2, FWIW

@PalleZingmark
Copy link
Contributor Author

PalleZingmark commented Oct 30, 2017

Updated tests according to the approach discussed in #409.
Here's my thoughts on the tests:

describe( 'Set Config should:', function() {
    process.argv[2] = '-c'
    process.argv[3] = '.stylintrc'
    var testMethod = app.setConfig( '.stylintrc' )
    var testConfig = app.setConfig( process.cwd() + '/.stylintrc' )

    it( 'return complete config when not complete config is passed in', function() {
        assert.ok( testConfig.reporter )
    } )

    it( 'update config state if passed a valid path', function() {
        assert.equal( Object.keys( testConfig ).length, Object.keys( testMethod ).length )
    } )

    it( 'throw if passed invalid path', function() {
        assert.throws(
            app.setConfig,
            TypeError,
            'setConfig err. Expected string, but received: ' + typeof dir
        )
    } )
} )

testMethod: Contains a config generated based on the default config options.
testConfig: Contains a config for test, generated on a .stylintrc that doesn't contain a full set of options.

Test 1

it( 'return complete config when not complete config is passed in', function() {
    assert.ok( testConfig.reporter )
} )

setConfig() should return a config with the reporter-option included even though it's not part of the actual .stylintrc file passed to the function.

Test 2

it( 'update config state if passed a valid path', function() {
    assert.equal( Object.keys( testConfig ).length, Object.keys( testMethod ).length )
} )

Make sure setConfig returns just as many options as testMethod even though the actual .stylintrc file passed to the function contains fever options.

@PalleZingmark PalleZingmark changed the title Set groupOutputByFile to default true if missing in config getConfig will assign default options to generated config Oct 30, 2017
Copy link
Collaborator

@emmabukacek emmabukacek left a comment

Choose a reason for hiding this comment

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

Thanks for your PR, @PalleZingmark ! Just a few, belated comments on this PR for your tests.

var testConfig = JSON.parse( stripJsonComments( fs.readFileSync( process.cwd() + '/.stylintrc', 'utf-8' ) ) )
var testConfig = app.setConfig( process.cwd() + '/.stylintrc' )

it( 'return complete config when not complete config is passed in', function() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice test! Can we paraphrase a bit to avoid ambiguity with what complete means here?

Perhaps something like:

it( 'return config with all properties when passed config with missing properties', function() {

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, I'll update the PR. Thnx.


it( 'update config state if passed a valid path', function() {
assert.deepEqual( testMethod, testConfig )
assert.equal( Object.keys( testConfig ).length, Object.keys( testMethod ).length )
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are we changing this test? If we must change this, we should still check for deep equality between two objects since doing key comparisons can lead to false positives.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Trying to remember this one.. 😣

I think I changed it since the important part is that they should contain the same amount of options now, not the exact same values since they may vary. I'll try to dig out my old notes.

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.

None yet

3 participants