Fix syntax detecting (#99)

[tonyganch]

Get file's extension and check if it's present in a list of supported syntaxes.
If not, print an error.

[mishanga]

Can you write some tests on it?

[tonyganch]

I wrote tests for our public methods: configure() + process...().
The only method left aside is processNode(): I think in 100% of cases it will be called from processString().

I have no idea how to test promises with mocha and without callbacks, so I used setTimeout.
Not perfect, but it works.
If anyone can show, how to make this part better, it will be great.

Tests for cli.js were not added in this branch, because the file was not changed.
However, I opened a new issue for this task: #105

[mishanga]

Тут странный индент. И у свойств тоже.

[tonyganch]

@mishanga, I know.
The way beautifiers work with scss and less should be improved, but it should be done in another branch.
I'll open another PR once it's ready.
Let's focus on this one.
The only purpose of this branch is to fix syntax detecting, nothing else.

[mishanga]

Okay.

[mishanga]

@anton-rudeshko can you help us with tests?

[anton-rudeshko]

@mishanga I'll look at this pr later this night.

[anton-rudeshko]

@tonyganch, could you please tell me what do you want to test here? I can't figure it out from sources.

[tonyganch]

@anton-rudeshko, there is a test/csscomb folder with a file for every supported syntax (css, scss, less) + one unsupported (txt).
I copy those files to a temp folder test/.csscomb, run comb.process...() for every file and compare the result with test/csscomb/*.expected files.
After the test is finished, remove temp folder.

Since all those methods are async, I added setTimeout to wait until they're done.

[anton-rudeshko]

@tonyganch, okay it's clear. But your PR is about "Fixing syntax detection" and here you're trying to test the whole app end-to-end. This is slightly more than just syntax detection I think =) Why is that?

[anton-rudeshko]

For example: if csscomb.js eventually will start supporting SCSS and someone will change the way beautifiers work with it, your tests about "Syntax detection" will fail.

[tonyganch]

@anton-rudeshko, ah, maybe I've overreacted :)
The only things I really wanted to test were processFile(), processPath() and processString() with css, scss and less files.
But that's almost all methods that we've got and it felt kind of natural to add tests for remaining ones.

I need to check if function can take a path like a.scss, read the file and comb it correctly.
I understand that I wrote micro integral tests, but it does not seem bad at all, does it?
If the way beautifiers work changes, at least with scss and less files (and it definitely will), we'll just correct the tests, too.

[tonyganch]

@anton-rudeshko, answering "Why is that": #99.
I added sorting support for scss files but wrote incorrect regexp to detect file's syntax and my tests did not fail.
Also I tested styles only with processString() function and it was not noticed that other methods got broken.
So I'd rather be on the safe side now with too many tests for such a small code change.

[anton-rudeshko]

I actually suggest you to extract your syntax check to method (private, I suppose) and test it synchronously.
About #99, I agree that there should be integration tests too, but they should be clearly separated from unit tests. I suggest to introduce new top level directory int-tests.
About your original question: you may use promise always method to pass callbacks and call done there. But I still strongly disagree with the structure of your tests.
Thanks
P.S. BTW nice rabbit out there. Makes me smile each time I write to you.

[tonyganch]

@anton-rudeshko, so you suggest that I move this line:

if (_this.SUPPORTED_SYNTAXES.indexOf(syntax) < 0)

to a private method?
Only to be able to run a test?
Are you serious?

you may use promise always method to pass callbacks

Any links to documentation, please?

@mishanga, I don't want to suggest skipping tests for this pr and make everything as @anton-rudeshko wants it to be in another branch, but it starts to look too complicated for a "small" fix to me.
If anyone of you have a strong opinion on tests structure (I do not), please, intervene, you are more than welcome.
I trust you with this decision and will support anything you do.
The branch is all yours.

@anton-rudeshko, maybe you will be interested in those issues, too: #89, #90.
It will be helpful to hear your opinion.

P.S. BTW thank you.

[anton-rudeshko]

@tonyganch
I'm talking about isolating your syntax check code from other parts to make it easy to test. I saw this line in your previous version:

// TODO: Move extension check into `_shouldProcess` method

That would be enough to write unit tests just for this check.

I mean Comb#processFile(() returns a promise (sadly, not always) and you can attach your check to Vow#always() instead of setTimeout():

comb[method](tempDir + file).always(function() {
  // check
  done();
});

After all I said @mishanga is still the owner of the project, so he is in charge to make decisions =)

[tonyganch]

@anton-rudeshko, wow, that's more clear, thank you!

The thing that makes me hesitate about shouldProcess is that it's used both for directories and files, while syntax check should be called for files only.
Also shouldProcess method is now called both inside processDirectory and processFile methods, what makes it run twice for a file in a folder.

So, after all, will the following changes satisfy anyone?

1. Remove all those tests from this branch, open a new issue and decide how to write integral tests there.

2. Make two separate shouldProcess methods. The one for a directory (shouldProcessDirectory, for example) will check for match with exclude only (as it does now), while shouldProcessFile will check for exclude + syntax match.

3. Replace this (processDirectory part):

if (_this._shouldProcess(fullname)) {
    return vfs.stat(fullname).then(function(stat) {
        if (stat.isDirectory()) {
            return _this.processDirectory(fullname);
        } else if (fullname.match(/\.css$/)) {
            return vow.when(_this.processFile(fullname)).then(function(errors) {
                if (errors) return errors;
            });
        }
    });
}

with something like this:

return vfs.stat(fullname).then(function(stat) {
    if (stat.isDirectory()) {
        return _this.shouldProcessDirectory(fullname) && _this.processDirectory(fullname);
    } else {
        return vow.when(_this.processFile(fullname)).then(function(errors) {
            if (errors) return errors;
        });
    }
});

And this (processFile part):

if (this._shouldProcess(path) && path.match(/\.[css, scss]$/)) {

with this:

if (this._shouldProcessFile(path)) {

4. Add ordinary sync tests for two shouldProcess... methods.

[anton-rudeshko]

1. Agree
2. Agree
3. Agree
4. Agree

=) Thanks

[tonyganch]

Done.
_shouldProcess() is untouched, _shouldProcessFile() runs syntax check first, then calls _shouldProcess()

@anton-rudeshko, thanks a lot!

[anton-rudeshko]

@tonyganch, looks much better and simpler now =) Thank you for your patience.

[mishanga]

@tonyganch 🚢

[tonyganch]

@mishanga, ping