ESLint

[jaydson]

Based on this tweet, i think we can soon implement ESLint for code validation. ESLinit will support all of ES6 features.

[UltCombo]

Nice!

[jonatanrdsantos]

Cool :+1:

[UltCombo]

Or perhaps we can use babel-eslint https://mobile.twitter.com/geteslint/status/573691550841991169 https://github.com/babel/babel-eslint

[jaydson]

That makes sense.

[UltCombo]

Yeah, I've started playing around in slush-es20xx with gulp-eslint using babel-eslint as the parser, and it looks pretty good so far. Looks like it is able to replace JSHint and JSCS entirely.

@jaydson I can land it on Harmonic in the next few days, but I'll need you to set the eslint rules, seeing as the code style repository looks outdated.

[jaydson]

Ok, count on me. @leobalter, can you help us?

[leobalter]

I'm using JSHint with the esnext option on Goiabada - which is entirely written in ES6, it is running flawlessly.

JSHint is much faster as it checks the code without using an AST parser, ESLint uses Esprima 2.0 to parse.

JSHint is supporting almost everything and if you find any missing feature, you can file an issue and it will be fixed probably in the average time of 48 hours. ESLint issues will need to wait a Esprima patch.

[UltCombo]

ESLint is fully pluggable (both parser and rules) which is a big plus. This allows using the Babel parser which is 100% compatible with Babel-generated code, including experimental features such as async functions. Using a single tool for linting also simplifies the workflow quite a bit.

[leobalter]

AFAIK, Babel are different ESLint tools for different tasks.

How would you use them together? You say you would use Babel inside ESLint to build something?

Using a single tool for linting also simplifies the workflow quite a bit.

That's exactly why I'm suggesting to keep JSHint.

[UltCombo]

@leobalter I meant using the babel-eslint parser in ESLint. As the docs say, you just have to install the package and set "parser": "babel-eslint" in the .eslintrc file. I've tested this with gulp-eslint and it works fine as well.

Using a single tool for linting also simplifies the workflow quite a bit.

That's exactly why I'm suggesting to keep JSHint.

Actually, we use JSHint for linting and JSCS for code style (although these are currently disabled in this project). ESLint has both linting and code style rules, which can mostly replace both JSHint and JSCS.

[UltCombo]

Actually, seeing as ESLint is fully pluggable, we can install/develop ESLint plugins for any missing JSHint/JSCS functionality, so I'm confident that it can easily replace JSHint and JSCS.

[UltCombo]

Also, consider my biggest concern (as expressed in this other thread):

[...] But it got me thinking, if we are going to be using the latest bleeding edge ECMAScript.next features in the future, and these features keep landing first on transpilers than on linters, this would get us into an infinite loop where our code can never be linted due to linters being behind transpilers regarding ECMAScript.next support.

Using ESLint with the babel-eslint parser completely obliterates this concern, since we use Babel for compiling ES.next code and babel-eslist can lint all code that Babel can compile.

Letting alone that, at a quick glance, (imho) ESLint has a far better infrastructure than JSHint and JSCS -- it is a breeze to develop a plugin which analyzes possible mistakes and code style based on AST.

The ESLint community also seems very responsive, and condensing/standardizing the linting logic can only be for the best -- not only the linting settings are in a single place and follow the same patterns, I believe it is also more user-friendly to display all code errors/warnings in a single place rather than scattering it across two linters.

As an extra plus, gulp-eslint has a very nice and flexible API (in contrast to gulp-jscs which takes ages to land even a basic feature such as reporters).

As far as I can see, we can only benefit from this change.

@leobalter WDYT?

[UltCombo]

No reply in the last 7 days, hence I assume agreement (or no opposition, at least).

I'll land ESLint on slush-es20xx in the next few days (probably around the weekend). Integrating ESLint itself is very easy, but I also want to make the incremental build workflow the less painful possible.

This means, while linting rules such as disallowing unreachable code, unused declarations and constant conditions should indeed fail a prepublish build, these cases commonly happen during development/debugging phases and should not fail an incremental build. Hence I'm considering to downgrade these rules to warnings in watch mode exclusively, this way it is possible to inform the user that there is some possible mistake on their part without breaking the current incremental build -- this can be worked in parallel with es6rocks/slush-es20xx#15 to add persistence to those warnings.

Or perhaps I'm overthinking this, and we could just do the linting after transpiling and unit testing. I have yet to evaluate the pros and cons of this approach.

[UltCombo]

I guess it is more logical run unit tests first and then lint, as it makes more sense to fix an issue first and only then care about code style/minor mistakes, instead of being pestered by code style/minor mistakes with code that doesn't even work.

A counterargument is that linting can help finding potential errors, such as the cause of an unit test fail, hence linting only when unit tests succeed could be counterproductive. Another issue is when you have a repository in an unit test failing state that can't be fixed immediately, which means the maintainers would have to edit the unit tests to skip the failing ones (and thus having an incorrect unit tests passing status) or have no linting at all.

Perhaps it is better to always execute both linting and unit tests in each incremental build, independent of any of them failing.

[jaydson]

It seems like @UltCombo from the future is having a conversation with @UltCombo from the past. Nice one :p

Now seriously. Go ahead with slush-es2xx and let's integrate it in Harmonic.

Perhaps it is better to always execute both linting and unit tests in each incremental build, independent of any of them failing.

:+1:

[UltCombo]

Hahah yeah. I've been working overtime in the last 2-3 weeks, so I haven't had much time to update the workflow yet. Anyway, I've pushed the initial implementation of ESLint (replacing JSHint and JSCS) on slush-es20xx: https://github.com/es6rocks/slush-es20xx/commit/916725e8671e405ef77f0aa7494c5bf207b1871c

I'll improve the workflow according to the comments above around the weekend. Until then, it would be nice if you could elaborate a .eslintrc file with suitable rules for Harmonic, @jaydson. You can use slush-es20xx's .eslintrc file as a starting point and check the ESLint docs to add/change its rules.

[UltCombo]

Still in the workflow improvements topic, another pain point is Mocha, or more specifically, the Mocha API.

Although Mocha is an excellent test framework for CLI usage, its API is completely and utterly crap. And this reflects in gulp-mocha, which is just a pile of ugly hacks around the Mocha API to try to make it usable. Likewise, I've spent several days wrapping gulp-mocha with more hacks to try to make it play well with our incremental build strategy, and the result is very ugly and far from perfect.

Perhaps it is worth taking a look at Jasmine.

[jaydson]

The basic rule of software development: Create a work around layer to solve a work around layer :hankey:

Now, seriously. What about Jest? Or even QUnit?

PS: With QUnit i'm sure we'll have full support from @leobalter :p And, the new QUnit version seems to be promising.

[UltCombo]

I just noticed Jasmine does not have a built-in equivalent of Mocha's --bail yet (https://github.com/jasmine/jasmine/issues/414), there are some hacks around (https://www.npmjs.com/package/jasmine-bail-fast) but yes, I'd rather look for another test framework than start off adding ugly hacks already.

I took a quick glance at Jest a while ago, it seemed a little bit too magical, but that might as well be a good thing in these unit testing grounds. It is worth experimenting with.

Does QUnit support the Node.js environment? I can't seem to find any official release for Node.

[UltCombo]

Oh, I just found a recipe for QUnit Node.js testing, we could experiment with QUnit too. Although, it has been over a year and half since the last time I wrote tests in QUnit, I've only actually used it when writing patches to jQuery Foundation projects.

[UltCombo]

Another option is running Mocha (CLI) in a child process, but that adds 200~300ms per incremental build cycle. We have a good batch logic now, so I don't know if that would actually be a problem.

[UltCombo]

I'm currently considering to use gulp-shell to run the Mocha CLI. Although this theoretically makes the incremental build cycles take longer, there are quite a few pros to this approach:

• Complete isolation:
  • Avoid tampering with Node's require cache.
  • Avoid global state pollution: each run gets a fresh environment, so one run's leaked globals don't have side effects in future runs.
• Avoid domains:
  • Domains should not be used as a catch-all, as they leak references and hog memory that is never GC'd.
• And the best of all, avoid touching Mocha's :hankey: API!

I'm inclined to use Mocha in this manner. Mocha is the unit test framework I've been using for over a year, so I'm more used to it as well.

@jaydson Of course, we can still experiment and even change the test framework, as long as there are compelling reasons to do so.

[jaydson]

@UltCombo , as you've set up everything related to tests since the beginning, i feel comfortable on you deciding what is the best solution here.

My 2 cents:

• It looks like we may have a bunch of failure points (mocha, gulp, gulp-shell...), or am i wrong?
• It looks a little bit complicated just to run tests

But as i said, if you are comfortable with the solution, go ahead.

[UltCombo]

IMO the test framework itself doesn't matter that much, the important part is having tests. And the tests are something we should improve -- right now there are only very basic functionality tests, in an ideal world we should be able to test each tiny part of the codebase. Hopefully we will see improvements in this area while developing the Harmonic API.

It looks like we may have a bunch of failure points (mocha, gulp, gulp-shell...), or am i wrong?

Mocha and gulp-shell are not very different from what you used to do with grunt-shell. :stuck_out_tongue_closed_eyes: That is, they just automate the process of running the mocha CLI. And gulp itself is just a very tiny core, all the magic is done by the gulp plugins which are basically transform streams.

But yes, I'm in the process of simplifying slush-es20xx as much as I can -- I've already dropped lazypipe, and now I'm removing the domains. Also, slush-es20xx's 2.0 branch already has a much better npm run dev -- it now successfully starts the watch task even when there are errors in the initial lint/compile/test run.

Once I'm done cleaning up es20xx, I'll start implementing the more ambitious ideas from https://github.com/es6rocks/slush-es20xx/issues/15 -- the incremental build's feedback will be far smarter and more useful.

About failure points:

• The incremental build (npm run dev) in the 2.0 branch uses gulp-plumber, which is a kind of "catch-all". I'll soon work in improving the feedback about what files in the project contain errors.
• The build task (prebublish) is slightly lower priority at the moment, it will break and fail the task in the first error (lint, compile or test) but I believe that is not really a problem -- this task is usually run after making sure everything is okay through the development mode (npm run dev), and this task's main purpose is actually to fail when there is something wrong, in order to prevent publishing broken builds.

@jaydson not sure if I addressed all of your concerns, feel free to ask if you have any doubts still. :smile:

[leobalter]

I'm gonna answer tomorrow. Short story: QUnit works on node, but the stdout logger is in a PR basically ready to land and release a new version.

The api is opinionated and simple. And yes, I can offer all the needed support.

On Mar 24, 2015, at 1:07 AM, Fabrício Matté notifications@github.com wrote:

IMO the test framework itself doesn't matter that much, the important part is having tests. And the tests are something we should improve -- right now there are only very basic functionality tests, in an ideal world we should be able to test each tiny part of the codebase. Hopefully we will see improvements in this part while developing the Harmonic API.

It looks like we may have a bunch of failure points (mocha, gulp, gulp-shell...), or am i wrong?

Mocha and gulp-shell are not very different from what you used to do with grunt-shell. That is, they just automate the process of running the mocha CLI. And gulp itself is just a very tiny core, all the magic is done by the gulp plugins which are basically transform streams.

But yes, I'm in the process of simplifying slush-es20xx as much as I can -- I've already dropped lazypipe, and now I'm removing the domains. Also, slush-es20xx's 2.0 branch already has a much better npm run dev -- it now successfully starts the watch task even when there are errors in the initial lint/compile/test run.

Once I'm done cleaning up es20xx, I'll start implementing the more ambitious ideas from es6rocks/slush-es20xx#15 -- the incremental build's feedback will be far smarter and more useful.

About SPOFs:

The incremental build (npm run dev) in the 2.0 branch uses gulp-plumber, which is a kind of "catch-all". I'll soon work in improving the feedback about what files in the project contain errors.

The build task (prebublish) is slightly lower priority at the moment, it will break and fail the task in the first error (lint, compile or test) but I believe that is not really a problem -- this task is usually run after making sure everything is okay through the development mode (npm run dev), and this task's main purpose is actually to fail when there is something wrong, in order to prevent publishing broken builds.

@jaydson not sure if I addressed your concerns, feel free to ask if you have any doubts still.

— Reply to this email directly or view it on GitHub.

[leobalter]

There's an almost done PR on QUnit to set a stdout reporter: https://github.com/jquery/qunit/pull/790

I'll follow that PR with also a TAP reporter.

Today QUnit works on node, it only misses a reporter for node, which is solved on plugins like node-qunit (which can be used as the current qunit module).

[UltCombo]

Thanks for the info @leobalter.

My main concern regarding test frameworks at the moment is whether/how they can be integrated inside of an incremental build environment. As far as I can see, there is no clean way to integrate a test framework through API usage inside of an incremental build (see my concerns regarding gulp-mocha above).

@leobalter if QUnit provides a CLI, then it could be integrated in a similar manner as I've described above. Of course, even without a proper CLI it is still easy to create a Node.js script to wrap the tests and exit with a non-zero exit status when assertion failures are detected.

As I have commented previously, changing the test framework is not a high priority in my opinion, but feel free to experiment if you are willing to.

[UltCombo]

ESLint and improved unit tests are already available in es20xx's master branch. I still want to pack in a couple more features before releasing slush-20xx v2.0.0. We could merge es20xx's master into Harmonic now for early testing and configuring the ESLint rules.

[jaydson]

Great job @UltCombo !

[UltCombo]

The new workflow has been implemented a while back, and there has only been positive feedback so far.

Closing, please open new issues to discuss specific topics.

[jaydson]

:+1: