Drop babel #2134

Merged
merged 24 commits into from Nov 30, 2016

Projects

None yet

6 participants

@davidtheclark
Contributor

Ref #2133.

Experimenting with using as much automation as possible to drop Babel.

What I've done so far is to use a limited number of Babel plugins, covering syntax not permitted by Node 4, to overwrite existing files in src/. This looks like it might be a suitable starting point.

Modules still exist, because that would have gotten nasty — I think those might require the most manual labor ... we'll see.

I'll toy with this a little bit more later today ...

@davidtheclark
Contributor

Interesting (?) problem here --- apparently non-babel tape is significantly slower than babel-tape in our 20000+ test situation? (ಥ﹏ಥ)

@davidtheclark
Contributor
davidtheclark commented Nov 28, 2016 edited

From my perspective, the only thing standing in the way of this PR, and dropping Babel, is the surprising slowness of non-Babel Tape tests in comparison to the babel-tape-runner tests we had going (and a little bit of cleanup). Tests are passing, things look ok — we can gradually re-adjust code style after Babel's changes if there's anything we disagree with. (Also, we'll probably need to put some more work into the ESLint config.)

Does anybody else see any problems?

@davidtheclark
Contributor

An experimental attempt at parallelizing rule tests with Tape seems to have reduced that test time back to something reasonable.

@davidtheclark davidtheclark referenced this pull request Nov 28, 2016
Closed

Drop Babel #2133

@davidtheclark
Contributor

@stylelint/core If we want to follow through with this (I definitely do want to), we should try to merge it ASAP, preferably before we merge other PRs to avoid annoying conflicts — since pretty much any changes will conflict with this.

Here's how you can help:

  • Pull down the branch and browse through the files. If you see things you don't like after Babel compiled the code, post about them and we can take commits directly into this branch.
  • Find loose ends and tie them up. There are probably some references to src/ that should now be lib/. There are probably some relics of the build process that we should remove. Nothing should point to dist/ anymore. Things like that.
  • Help explore the too-many-tests conundrum. Can we get Jest to work for us? Or should we try to make a custom script (as started here) that runs Tape rule tests in parallel for faster CI builds? Right now that script does not work with Windows and there's no "development" version, if we need one ...
  • Give a 👍 if/when you think changes are looking ready to merge.
@evilebottnawi
Member
evilebottnawi commented Nov 28, 2016 edited

@davidtheclark Seem very good 👍 Down coverage but I think to go out 8 versions, we can bring it back to 100 percent. Also we can write CLI tests without trouble now.

@jeddy3
Member
jeddy3 commented Nov 28, 2016

This looks very good to me!

apparently non-babel tape is significantly slower than babel-tape in our 20000+ test situation?

It behaves more predictably for me on my local machine. tape took 8 seconds to plough through the 23000 /rules/ tests (in this branch), whereas babel-tape-run took 20 seconds (on master) on the first run, and then 8 seconds on each subsequent run... I assume because babel-tape-run is caching the transpiled files after the 1st run. Oddly, the new parallel script (in this branch) took 12 seconds...

Can we get Jest to work for us?

I've tried to investigate the jest slowness further, but the /rules/ tests still run, on my local machine, at over a minute without -i and crash with that memory error with -i.

Hopefully something we emerge from your issue in the jest repo that will point to a solution.

Pull down the branch and browse through the files.

The changes don't look drastic at all and I guess that's because of node@4's support for arrow functions, const, Maps, Sets etc!

we should try to merge it ASAP,

Agreed. We can handle any clean-up afterwards.

@davidtheclark
Contributor

@jeddy3 Are you on Node 6 or 7? There might be some significant between Tape speed on those and on Node 4 — I'm on Node 4, and the Travis tests on Node 4 and 6 seemed to be the worst culprits.

@davidtheclark
Contributor

I will try to get this merged in tonight. We just have to make sure we all poke around in it before a release.

@ntwb
Member
ntwb commented Nov 29, 2016

Picking some recent and random Travis CI stats:

https://travis-ci.org/stylelint/stylelint/builds/179402560
 5565.1 Node.js: 6 CI=lint+flow 1 min 30 sec
 5565.2 Node.js: 6 CI=jest 1 min 51 sec
 5565.3 Node.js: 7 CI=test 1 min 37 sec
 5565.4 Node.js: 6 CI=test 1 min 54 sec
 5565.5 Node.js: 4 CI=test 3 min

https://travis-ci.org/stylelint/stylelint/builds/179258349
 5545.1 Node.js: 6 CI=lint+flow 1 min 50 sec
 5545.2 Node.js: 6 CI=jest 2 min 27 sec
 5545.3 Node.js: 7 CI=test 2 min 15 sec
 5545.4 Node.js: 6 CI=test 2 min 1 sec
 5545.5 Node.js: 4 CI=test 3 min 45 sec

https://travis-ci.org/stylelint/stylelint/builds/179121672
 5518.1 Node.js: 6 no environment variables set 1 min 35 sec
 5518.2 Node.js: 6 no environment variables set 1 min 24 sec
 5518.3 Node.js: 7 no environment variables set 1 min 47 sec
 5518.4 Node.js: 6 no environment variables set 1 min 46 sec
 5518.5 Node.js: 4 no environment variables set 2 min 45 sec

https://travis-ci.org/stylelint/stylelint/builds/179134801
 5525.1 Node.js: 6 CI=lint+flow 1 min 47 sec
 5525.2 Node.js: 6 CI=jest 1 min 52 sec
 5525.3 Node.js: 7 CI=test 2 min 8 sec
 5525.4 Node.js: 6 CI=test 2 min 29 sec
 5525.5 Node.js: 4 CI=test 2 min 37 sec

Compared to this PR:

https://travis-ci.org/stylelint/stylelint/builds/179349067
 5562.1 Node.js: 6 CI=lint+flow 1 min 5 sec
 5562.2 Node.js: 6 CI=jest 58 sec
 5562.3 Node.js: 7 CI=test 55 sec
 5562.4 Node.js: 6 CI=test 57 sec
 5562.5 Node.js: 4 CI=test 1 min 53 sec

So, as far as Travis CI is concerned all build jobs are faster now (based on my random sampling)

💥

@ntwb
Member
ntwb commented Nov 29, 2016

npm test here locally with NodeJS 7.2.0 is 12.53 seconds

@ntwb
Member
ntwb commented Nov 29, 2016

One issue I am seeing is rather than seeing the final summary:

1..23886
# tests 23886
# pass  23886

# ok

I'm seeing numerous instances of the test counts resetting 1:

For example:

ok 2513 ignore constructs should be accepted
# 
> rule: selector-combinator-space-before
> config: "always"
> code: "a  +a {}"

ok 2514 two spaces before + combinator should register one warning
ok 2515 two spaces before + combinator should warn with message Expected single space before "+" (selector-combinator-space-before)

1..2515
# tests 2515
# pass  2515

# ok


TAP version 13
# 
> rule: selector-descendant-combinator-no-non-space
> config: true
> code: ""

ok 1 empty stylesheet should be accepted
# 
> rule: selector-descendant-combinator-no-non-space
> config: true
> code: "a {}"

In the end there are 20 occurrences where passed test count resets to 1:

1..730
# tests 730
# pass  730

# ok
...
1..637
# tests 637
# pass  637

# ok
...
1..971
# tests 971
# pass  971

# ok
...
1..1007
# tests 1007
# pass  1007

# ok
...
1..855
# tests 855
# pass  855

# ok
...
1..685
# tests 685
# pass  685

# ok
...
1..1426
# tests 1426
# pass  1426

# ok
...
1..1202
# tests 1202
# pass  1202

# ok
...
1..1208
# tests 1208
# pass  1208

# ok
...
1..1354
# tests 1354
# pass  1354

# ok
...
1..762
# tests 762
# pass  762

# ok
...
1..984
# tests 984
# pass  984

# ok
...
1..1087
# tests 1087
# pass  1087

# ok
...
1..777
# tests 777
# pass  777

# ok
...
1..1050
# tests 1050
# pass  1050

# ok
...
1..1512
# tests 1512
# pass  1512

# ok
...
1..1665
# tests 1665
# pass  1665

# ok
...
1..2515
# tests 2515
# pass  2515

# ok
...
1..1813
# tests 1813
# pass  1813

# ok
...
1..921
# tests 921
# pass  921

# ok
@jeddy3
Member
jeddy3 commented Nov 29, 2016 edited

Are you on Node 6 or 7?

I'm on 7. You're right about the significant difference between 4, 6 and 7. While 7 was ploughed through at 8 seconds, both 4(.6.2) and 6(.9.1) took over 5 minutes on my local machine using just tape. They seem to storm through the first 10000 tests in a few seconds, and then plod through remaining. Using the parallel script on both 4(.6.2) and 6(.9.1) took about 30 seconds, which is a great improvement over 5 minutes! :)

@davidtheclark
Contributor

I'm seeing numerous instances of the test counts resetting 1

@ntwb That is because of the script that runs tape in separate processes. I think it's actually fine, though it might be nice to throw in an actual total at the end.

They seem to storm through the first 10000 tests in a few seconds

@jeddy3 Yeah, isn't that weird?

Anyway, I got swamped by work (returning from a short holiday) so could not finish this last night. Give me another day or two ...

@davidtheclark
Contributor

I got engrossed this morning in tinkering with the parallel tape script and made something fancy ... give it a try.

@jeddy3
Member
jeddy3 commented Nov 29, 2016

I got engrossed this morning in tinkering with the parallel tape script and made something fancy ... give it a try.

That's some fancy command line action :)

Performance still looks great on my machine (11 seconds on Node 7 and 30 seconds on Node 4 and 6). Output of failing tests is much clearer than the default tape output!

Performance on CI looks great too.

I think we're in a better position now than we were a week ago to see how things pan out, over the next few weeks, with the viability of using jest for rule testing. At the moment using jest for lib tests and system-tests (and possibly utils - see #2135), and using tape for rule tests seems like we're playing to the strengths of both runners.

@m-allanson
Member

This is looking 👍 to me. The only issue I noticed was with node v4 and the system tests.

Using node v4.6.2, npm run jest-system gives the following output:


 FAIL  system-tests/001/001.test.js
  ● Test suite failed to run

    SyntaxError: Unexpected token {

      at transformAndBuildScript (node_modules/jest/node_modules/jest-cli/node_modules/jest-runtime/build/transform.js:316:10)
      at process._tickCallback (node.js:385:9)

 FAIL  system-tests/002/002.test.js
  ● Test suite failed to run

    SyntaxError: Unexpected token {

      at transformAndBuildScript (node_modules/jest/node_modules/jest-cli/node_modules/jest-runtime/build/transform.js:316:10)
      at process._tickCallback (node.js:385:9)

 FAIL  system-tests/003/003.test.js
  ● Test suite failed to run

    SyntaxError: Unexpected token {

      at transformAndBuildScript (node_modules/jest/node_modules/jest-cli/node_modules/jest-runtime/build/transform.js:316:10)
      at process._tickCallback (node.js:385:9)

Test Suites: 3 failed, 3 total
Tests:       0 total
Snapshots:   0 total
Time:        0.27s
Ran all test suites matching //system-tests//.

The tests pass on nodes v6 and v7.

@ntwb
Member
ntwb commented Nov 30, 2016

Nice, and if I'm not mistaken 470cef0 will address that stylelint cli binary location in #1968

The only issue I noticed was with node v4 and the system tests.

NodeJS v6 entered LTS (Long TErm Support) 18th October 2016.

I'm trying to think of reasons to not drop NodeJS 4.x support?

That's some fancy command line action :)

Yes, it is, nice not having 23k lines of terminal output, will improve CI readability as only the failed test results are displayed 💥

@davidtheclark
Contributor

I'm trying to think of reasons to not drop NodeJS 4.x support?

@ntwb Node v4 will be actively supported until mid-2018, so I think it's very premature to consider dropping support for it.

@davidtheclark davidtheclark A couple more tweaks
324f523
@davidtheclark
Contributor

Ugh. Windows is still blocking this merge:

stylelint@7.6.0 tape C:\projects\stylelint
node scripts/rules-tape.js
�[?25lTesting ...
Error: spawn tape ENOENT
at exports._errnoException (util.js:1022:11)
at Process.ChildProcess._handle.onexit (internal/child_process.js:193:32)
at onErrorNT (internal/child_process.js:359:16)
at _combinedTickCallback (internal/process/next_tick.js:74:11)
at process._tickCallback (internal/process/next_tick.js:98:9)
�[?25h---------------|----------|----------|----------|----------|----------------|
File | % Stmts | % Branch | % Funcs | % Lines |Uncovered Lines |
---------------|----------|----------|----------|----------|----------------|
All files | 63.22 | 34.62 | 41.67 | 65 | |
Build success
rules-tape.js | 63.22 | 34.62 | 41.67 | 65 |... 116,117,121 |

@davidtheclark davidtheclark Hate Windows
debc495
@davidtheclark davidtheclark merged commit 43df9d6 into master Nov 30, 2016

3 of 4 checks passed

coverage/coveralls Coverage decreased (-3.7%) to 91.885%
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@davidtheclark davidtheclark deleted the drop-babel branch Nov 30, 2016
@nkabardin
Contributor

Fantastic work!

@ntwb
Member
ntwb commented Nov 30, 2016

Boom 💥 , thanks @davidtheclark

@jeddy3 jeddy3 changed the title from [WIP] Drop babel to Drop babel Nov 30, 2016
@jeddy3 jeddy3 added a commit to stylelint/stylelint-config-standard that referenced this pull request Dec 3, 2016
@karlhorky @jeddy3 karlhorky + jeddy3 Fix links due to stylelint/stylelint#2134 (#52) 013f4f2
@JaKXz JaKXz added a commit to JaKXz/stylelint-webpack-plugin that referenced this pull request Dec 18, 2016
@karlhorky @JaKXz karlhorky + JaKXz fix: stylelint formatter path (#49) 46baa59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment