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

ESLint failing tests #62

Open
Cellule opened this Issue Apr 6, 2015 · 24 comments

Comments

Projects
None yet
7 participants
@Cellule
Contributor

Cellule commented Apr 6, 2015

Hi, recently I have encountered quite a few problems with ESLint because I used babel-eslint. I would really like to keep using it for its better support for es6 and jsx.
I wanted to have an overview of the problems with babel-eslint so I executed ESLint test suite using babel-eslint and I wanted to share my results here.

The test suite found 115 failing test cases affecting 26 rules.
the rules are

  • block-scoped-var
  • brace-style
  • comma-spacing
  • complexity
  • consistent-return
  • default-case
  • indent
  • key-spacing
  • no-catch-shadow
  • no-div-regex
  • no-empty-class
  • no-ex-assign
  • no-extra-parens
  • no-fallthrough
  • no-inline-comments
  • no-multi-spaces
  • no-regex-spaces
  • no-shadow-restricted-names
  • no-undefined
  • padded-blocks
  • space-before-blocks
  • space-infix-ops
  • spaced-line-comment
  • valid-jsdoc
  • vars-on-top
  • wrap-regex

and you can see the detailed errors in the gist below (I enhanced a bit eslint-tester to have more information about failing tests)
https://gist.github.com/Cellule/37e1231437b1a28cdf46

I hope this can help solve most discrepancies between espree and babel-eslint.
If you want I can share how I ran the test suite since there is no official support at the moment.

@kittens

This comment has been minimized.

Show comment
Hide comment
@kittens

kittens Apr 8, 2015

Member

If you want I can share how I ran the test suite since there is no official support at the moment.

Sure, this would be great. Bit hard to ensure that these issues actually get fixed unless I can repeatedly run the test suite.

Member

kittens commented Apr 8, 2015

If you want I can share how I ran the test suite since there is no official support at the moment.

Sure, this would be great. Bit hard to ensure that these issues actually get fixed unless I can repeatedly run the test suite.

@Cellule

This comment has been minimized.

Show comment
Hide comment
@Cellule

Cellule Apr 8, 2015

Contributor

Right now I am working with eslint-tester to provide a way to change the config for all test cases https://github.com/eslint/eslint-tester/issues/19

However, in order to run the tests, I simply added a field on the ESlintTester prototype that would be used to set the parser for the tests.
Then I installed babel-eslint in the ESlint repo and I set the new ESlintTester field in tests\lib\rules.js to babel-eslint like

var ESLintTester = require("eslint-tester");
ESLintTester.defaultParser = "babel-eslint";

Then every test ran after that point would use babel-eslint instead of espree

Contributor

Cellule commented Apr 8, 2015

Right now I am working with eslint-tester to provide a way to change the config for all test cases https://github.com/eslint/eslint-tester/issues/19

However, in order to run the tests, I simply added a field on the ESlintTester prototype that would be used to set the parser for the tests.
Then I installed babel-eslint in the ESlint repo and I set the new ESlintTester field in tests\lib\rules.js to babel-eslint like

var ESLintTester = require("eslint-tester");
ESLintTester.defaultParser = "babel-eslint";

Then every test ran after that point would use babel-eslint instead of espree

@gabrielecirulli

This comment has been minimized.

Show comment
Hide comment
@gabrielecirulli

gabrielecirulli Apr 14, 2015

Please don't quote me on this, but might it be a change in the eslint api? I noticed this started to happen when eslint went from 0.16 to 0.17. We just locked the version to 0.16 and everything was fine, but then babel-eslint got update and the error got triggered again. It also happens on eslint 0.19.

gabrielecirulli commented Apr 14, 2015

Please don't quote me on this, but might it be a change in the eslint api? I noticed this started to happen when eslint went from 0.16 to 0.17. We just locked the version to 0.16 and everything was fine, but then babel-eslint got update and the error got triggered again. It also happens on eslint 0.19.

@gabrielecirulli

This comment has been minimized.

Show comment
Hide comment
@gabrielecirulli

gabrielecirulli Apr 14, 2015

In my case the main problem seems to be recognizing that JSX is using certain variables. It will trigger no-unused-var for all JSX elements.

gabrielecirulli commented Apr 14, 2015

In my case the main problem seems to be recognizing that JSX is using certain variables. It will trigger no-unused-var for all JSX elements.

@gabrielecirulli

This comment has been minimized.

Show comment
Hide comment
@gabrielecirulli

gabrielecirulli Apr 14, 2015

Locking to the following versions: eslint@0.16.2 babel-eslint@2.0.2 seems to temporarily remove the issue.

gabrielecirulli commented Apr 14, 2015

Locking to the following versions: eslint@0.16.2 babel-eslint@2.0.2 seems to temporarily remove the issue.

@jsg2021

This comment has been minimized.

Show comment
Hide comment
@jsg2021

jsg2021 May 15, 2015

+1 for this

jsg2021 commented May 15, 2015

+1 for this

@hzoo hzoo added the enhancement label May 15, 2015

@hzoo

This comment has been minimized.

Show comment
Hide comment
@hzoo

hzoo May 15, 2015

Member

@Cellule I'm probably just bad at following your instructions (and haven't been able to get this working myself) - have you been able to get farther on this? It definitely would be awesome to be able to run against all ESLint tests.

Member

hzoo commented May 15, 2015

@Cellule I'm probably just bad at following your instructions (and haven't been able to get this working myself) - have you been able to get farther on this? It definitely would be awesome to be able to run against all ESLint tests.

@Cellule

This comment has been minimized.

Show comment
Hide comment
@Cellule

Cellule May 16, 2015

Contributor

The problem is that my change to have a default configuration in eslint-tester hasn't been approved yet. see eslint/eslint-tester#20

So what you can do is clone eslint then run npm i cellule/eslint-tester#default_config babel-eslint to have my version.
Then go to tests\lib\rules\block-scoped-var.js (the first rule) and add at the top of the file

var ESLintTester = require("eslint-tester");
ESLintTester.setDefaultConfig({
    parser: "babel-eslint"
});

and then run npm test.

It's not very clean, but it gets the job done.
As of today I found 77 tests cases failing

Contributor

Cellule commented May 16, 2015

The problem is that my change to have a default configuration in eslint-tester hasn't been approved yet. see eslint/eslint-tester#20

So what you can do is clone eslint then run npm i cellule/eslint-tester#default_config babel-eslint to have my version.
Then go to tests\lib\rules\block-scoped-var.js (the first rule) and add at the top of the file

var ESLintTester = require("eslint-tester");
ESLintTester.setDefaultConfig({
    parser: "babel-eslint"
});

and then run npm test.

It's not very clean, but it gets the job done.
As of today I found 77 tests cases failing

@hzoo

This comment has been minimized.

Show comment
Hide comment
@hzoo

hzoo May 16, 2015

Member

Ok I'l try it out tomorrow - and looks like there's less tests failing!

Member

hzoo commented May 16, 2015

Ok I'l try it out tomorrow - and looks like there's less tests failing!

@Cellule

This comment has been minimized.

Show comment
Hide comment
@Cellule

Cellule May 16, 2015

Contributor

If you want you can try my branch
https://github.com/Cellule/babel-eslint/tree/eslint_tests
to use it, you need to have a copy of eslint and run npm link in eslint directory. Then run npm link eslint in babel-eslint directory. That way you would have access to the test files.
After that, npm test in babel-eslint will run the test suite of eslint.

It's not perfect, I've done this real quick tonight and there are some false negative (some tests also fails with espree), but at least you can see some tests with minimal setup.

Contributor

Cellule commented May 16, 2015

If you want you can try my branch
https://github.com/Cellule/babel-eslint/tree/eslint_tests
to use it, you need to have a copy of eslint and run npm link in eslint directory. Then run npm link eslint in babel-eslint directory. That way you would have access to the test files.
After that, npm test in babel-eslint will run the test suite of eslint.

It's not perfect, I've done this real quick tonight and there are some false negative (some tests also fails with espree), but at least you can see some tests with minimal setup.

@ilyavolodin

This comment has been minimized.

Show comment
Hide comment
@ilyavolodin

ilyavolodin May 29, 2015

Just to let you guys know, I just published new version of eslint-tester that includes ability to provide default configuration, including parser that @Cellule submitted.

ilyavolodin commented May 29, 2015

Just to let you guys know, I just published new version of eslint-tester that includes ability to provide default configuration, including parser that @Cellule submitted.

@hzoo

This comment has been minimized.

Show comment
Hide comment
@hzoo

hzoo May 29, 2015

Member

Ok thanks @ilyavolodin (and @Cellule), I'l check it out. So I need to be able to run somewhere

var ESLintTester = require("eslint-tester");
ESLintTester.setDefaultConfig({
    parser: "babel-eslint"
});

and then run all the ESLint tests? What do you guys think would be the best way to integrate the tests into the project so we can test against regressions as well as fix the already failing tests? - ESLint is already a devDependency

Member

hzoo commented May 29, 2015

Ok thanks @ilyavolodin (and @Cellule), I'l check it out. So I need to be able to run somewhere

var ESLintTester = require("eslint-tester");
ESLintTester.setDefaultConfig({
    parser: "babel-eslint"
});

and then run all the ESLint tests? What do you guys think would be the best way to integrate the tests into the project so we can test against regressions as well as fix the already failing tests? - ESLint is already a devDependency

@Cellule

This comment has been minimized.

Show comment
Hide comment
@Cellule

Cellule May 29, 2015

Contributor

The problem is that having eslint as a dependency doesn't come with the tests. So right now no matter how you look at it you need to have your own copy of eslint and then run the tests. You can either modifiy eslint to run the tests with babel-eslint or use my (very hacky) branch of babel-eslint to run the eslint test suite (which is a bit less reliable).

Contributor

Cellule commented May 29, 2015

The problem is that having eslint as a dependency doesn't come with the tests. So right now no matter how you look at it you need to have your own copy of eslint and then run the tests. You can either modifiy eslint to run the tests with babel-eslint or use my (very hacky) branch of babel-eslint to run the eslint test suite (which is a bit less reliable).

@Cellule

This comment has been minimized.

Show comment
Hide comment
@Cellule

Cellule May 29, 2015

Contributor

Since no matter how you look at it, you will need a copy of eslint. I figured it would be simpler to fork eslint and run the tests.
So I made a branch for eslint with a fairly simple way to run eslint test suite with babel-eslint
you can see the diff here

Contributor

Cellule commented May 29, 2015

Since no matter how you look at it, you will need a copy of eslint. I figured it would be simpler to fork eslint and run the tests.
So I made a branch for eslint with a fairly simple way to run eslint test suite with babel-eslint
you can see the diff here

@hzoo

This comment has been minimized.

Show comment
Hide comment
@hzoo

hzoo May 29, 2015

Member

Looks simple enough - my idea was to use a git sub module or something

// 88 failing tests

Member

hzoo commented May 29, 2015

Looks simple enough - my idea was to use a git sub module or something

// 88 failing tests

hzoo added a commit to hzoo/babel-eslint that referenced this issue May 30, 2015

hzoo added a commit to hzoo/babel-eslint that referenced this issue May 30, 2015

hzoo added a commit to hzoo/babel-eslint that referenced this issue May 30, 2015

hzoo added a commit to hzoo/babel-eslint that referenced this issue May 31, 2015

hzoo added a commit to hzoo/babel-eslint that referenced this issue Jun 7, 2015

hzoo added a commit that referenced this issue Jun 8, 2015

Merge pull request #116 from hzoo/eslint-tests
Add eslint submodule to run eslint tests - Ref #62
@hzoo

This comment has been minimized.

Show comment
Hide comment
@hzoo

hzoo Jul 10, 2015

Member

@Cellule not really sure what we can do - a lot of the failing tests aren't an issue - strict mode, etc.

Member

hzoo commented Jul 10, 2015

@Cellule not really sure what we can do - a lot of the failing tests aren't an issue - strict mode, etc.

@Cellule

This comment has been minimized.

Show comment
Hide comment
@Cellule

Cellule Jul 11, 2015

Contributor

We're going to have to find a way to mark or disregard these tests we know are false negatives. Right now I don't have any idea since we don't want to edit the source code of the tests.

Contributor

Cellule commented Jul 11, 2015

We're going to have to find a way to mark or disregard these tests we know are false negatives. Right now I don't have any idea since we don't want to edit the source code of the tests.

@hzoo hzoo added the help wanted label Feb 16, 2016

@kaicataldo

This comment has been minimized.

Show comment
Hide comment
@kaicataldo

kaicataldo May 15, 2016

Member

Looking into this and this is the current output from the tests:


> babel-eslint@6.0.4 eslint /Users/kaicataldo/Code/babel-eslint
> cd eslint && mocha -c tests/lib/rules/*.js -r ../eslint-tester.js

Use babel-eslint for test suite

assert.js:90
  throw new assert.AssertionError({
  ^
AssertionError: Should have no errors but had 1: [ { fatal: true,
    severity: 2,
    message: 'Parsing error: \'with\' in strict mode',
    line: 1,
    column: 2 } ]
    at testValidTemplate (/Users/kaicataldo/Code/babel-eslint/eslint/lib/testers/rule-tester.js:324:20)
    at Function.<anonymous> (/Users/kaicataldo/Code/babel-eslint/eslint/lib/testers/rule-tester.js:405:21)
    at Function.RuleTester.it (/Users/kaicataldo/Code/babel-eslint/eslint/lib/testers/rule-tester.js:193:19)
    at /Users/kaicataldo/Code/babel-eslint/eslint/lib/testers/rule-tester.js:404:28
    at Array.forEach (native)
    at Function.<anonymous> (/Users/kaicataldo/Code/babel-eslint/eslint/lib/testers/rule-tester.js:403:24)
    at Function.RuleTester.describe (/Users/kaicataldo/Code/babel-eslint/eslint/lib/testers/rule-tester.js:189:19)
    at Object.RuleTester.run (/Users/kaicataldo/Code/babel-eslint/eslint/lib/testers/rule-tester.js:402:20)

Been trying to figure out where that comes from...Will keep digging

Member

kaicataldo commented May 15, 2016

Looking into this and this is the current output from the tests:


> babel-eslint@6.0.4 eslint /Users/kaicataldo/Code/babel-eslint
> cd eslint && mocha -c tests/lib/rules/*.js -r ../eslint-tester.js

Use babel-eslint for test suite

assert.js:90
  throw new assert.AssertionError({
  ^
AssertionError: Should have no errors but had 1: [ { fatal: true,
    severity: 2,
    message: 'Parsing error: \'with\' in strict mode',
    line: 1,
    column: 2 } ]
    at testValidTemplate (/Users/kaicataldo/Code/babel-eslint/eslint/lib/testers/rule-tester.js:324:20)
    at Function.<anonymous> (/Users/kaicataldo/Code/babel-eslint/eslint/lib/testers/rule-tester.js:405:21)
    at Function.RuleTester.it (/Users/kaicataldo/Code/babel-eslint/eslint/lib/testers/rule-tester.js:193:19)
    at /Users/kaicataldo/Code/babel-eslint/eslint/lib/testers/rule-tester.js:404:28
    at Array.forEach (native)
    at Function.<anonymous> (/Users/kaicataldo/Code/babel-eslint/eslint/lib/testers/rule-tester.js:403:24)
    at Function.RuleTester.describe (/Users/kaicataldo/Code/babel-eslint/eslint/lib/testers/rule-tester.js:189:19)
    at Object.RuleTester.run (/Users/kaicataldo/Code/babel-eslint/eslint/lib/testers/rule-tester.js:402:20)

Been trying to figure out where that comes from...Will keep digging

@hzoo

This comment has been minimized.

Show comment
Hide comment
@hzoo

hzoo May 15, 2016

Member

It's prob b/c babel-eslint has strict mode by default (since you are using modules, etc) and eslint uses with in it's tests

Member

hzoo commented May 15, 2016

It's prob b/c babel-eslint has strict mode by default (since you are using modules, etc) and eslint uses with in it's tests

@kaicataldo

This comment has been minimized.

Show comment
Hide comment
@kaicataldo

kaicataldo May 16, 2016

Member

@hzoo Ah, that makes sense...Hm, so how did they ever work?

Member

kaicataldo commented May 16, 2016

@hzoo Ah, that makes sense...Hm, so how did they ever work?

@hzoo

This comment has been minimized.

Show comment
Hide comment
@hzoo

hzoo May 16, 2016

Member

It didn't actually - there were always failing tests

Member

hzoo commented May 16, 2016

It didn't actually - there were always failing tests

@kaicataldo

This comment has been minimized.

Show comment
Hide comment
@kaicataldo

kaicataldo May 16, 2016

Member

It sounded like it was running some of the tests, though - or maybe I misunderstood. What I pasted above is the entire output at the moment - doesn't run any of them :(

Member

kaicataldo commented May 16, 2016

It sounded like it was running some of the tests, though - or maybe I misunderstood. What I pasted above is the entire output at the moment - doesn't run any of them :(

@hzoo

This comment has been minimized.

Show comment
Hide comment
@hzoo

hzoo May 16, 2016

Member

Yeah I guess it changed in the time since I ran them? It looks like it's just stopping on the first failing one rather than running it all

Member

hzoo commented May 16, 2016

Yeah I guess it changed in the time since I ran them? It looks like it's just stopping on the first failing one rather than running it all

@kaicataldo

This comment has been minimized.

Show comment
Hide comment
@kaicataldo

kaicataldo May 16, 2016

Member

I see - yeah, these error messages are not super helpful, unfortunately. When I set strict to false it reports a different parsing error with no real error message.

Member

kaicataldo commented May 16, 2016

I see - yeah, these error messages are not super helpful, unfortunately. When I set strict to false it reports a different parsing error with no real error message.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment