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

Update deps, fix tests #1254

Merged
merged 9 commits into from
Jan 19, 2019
Merged

Update deps, fix tests #1254

merged 9 commits into from
Jan 19, 2019

Conversation

ljharb
Copy link
Member

@ljharb ljharb commented Jan 3, 2019

master is broken. This PR updates dependencies, but it's still broken.

I suspect that fixing the tests will make this PR pass as well; I'm not clear, however, on how to do that.

…escript`, `gulp`, `linklocal`, `nyc`, `redux`, `rimraf`, `sinon`, `typescript-eslint-parser`
@coveralls
Copy link

coveralls commented Jan 3, 2019

Coverage Status

Coverage decreased (-0.05%) to 97.306% when pulling 64d9be7 on ljharb:travis into 1cd82eb on benmosher:master.

@benmosher
Copy link
Member

Bizarre disjoint test failure sets depending on which Node/ESLint pairing you look at in the Travis results. Almost stranger still that for ESLint 4 + Node 8&10, all tests pass.

I want to get back involved and start cleaning up soon. Hopefully I can take a look this week. I remember there being a TypeScript catch-22 but it's been months since I've looked at it. 😞

because Typescript parser can't deal
@ljharb
Copy link
Member Author

ljharb commented Jan 15, 2019

We shouldn't need to drop eslint 2/3; can we instead skip the TS tests on those eslint versions?

@benmosher
Copy link
Member

benmosher commented Jan 15, 2019

was curious what your reaction would be. yeah, I guess that is doable (to skip TS tests for v2/3). I had that briefly locally but I didn't love explicitly coupling the test content to ESLINT_VERSION and it's not obvious to me what alternative there is?

@ljharb
Copy link
Member Author

ljharb commented Jan 15, 2019

We could explicitly uninstall the typescript parser in travis.yml in v2 and v3, and then auto skip typescript tests whenever the parser isn’t try/catch requireable?

@benmosher
Copy link
Member

benmosher commented Jan 15, 2019

ooo that is an interesting idea.

I just pushed with them wrapped in a skipESLints helper and it works as well as expected.

things still failing:

  • the deprecated rule is broken with ESLint 5 on account of Breaking: remove attachComment option eslint/espree#394
  • no-amd + no-commonjs seem to be totally busted for reasons I don't yet understand. maybe something local, will see what happens on CI I bumped babel-eslint and that did it. something with ESLint 5 compatibility as well, I'm guessing (nothing passes the first check for module scope). so I reverted it back to babel-eslint v8

also left my debug console in :-(
@benmosher
Copy link
Member

another idea: more travis-set versions of packages in the build matrix.

i.e.

  • node4 eslint2 babel-eslint@whatever ts-eslint-parser@whatever
  • node6 eslint3 babel-eslint@whatever+2 ts-eslint-parser@whatever+4
    ...

@ljharb
Copy link
Member Author

ljharb commented Jan 15, 2019

It makes defining the matrix more annoying, but that’s a much better problem to have than “not testing something” or “tests failing” :-) I’m all for it

@benmosher
Copy link
Member

leaving myself a reminder to use these to fix deprecated: eslint/espree#394 (comment)

* technically espree v5
- also technically it doesn't support comma-first anymore but that seems reasonable
@benmosher
Copy link
Member

no-deprecated fixed. definitely seems like an expanded build matrix is in order. seems like the typescript parser is allergic to Node <= 6 now too

for tests against older ESLint versions.
@benmosher
Copy link
Member

getting different results on Travis than I am seeing locally. in particular, I don't see the same Node <= 6 failures so not sure what's going on there.

@benmosher
Copy link
Member

alright, winnowed down to just ESLint 4 / Node 4 matrix entry failing, still due to some nonsense with the typescript stuff.

I'm inclined to drop that entry as it doesn't seem super high-value or high-risk and this whole thing is blocking merging PRs / releasing features. do you want to take a look first @ljharb?

@ljharb
Copy link
Member Author

ljharb commented Jan 18, 2019

I’d say it’s fine to temporarily add it to allowed failures, but continue running it and pursue fixing it separately?

@benmosher
Copy link
Member

Ahhhh great idea, building now!

@benmosher
Copy link
Member

Thanks for starting this branch, @ljharb! I’m excited to have master clean to start merging and shipping PRs again!

package.json Outdated Show resolved Hide resolved
@ljharb
Copy link
Member Author

ljharb commented Jan 19, 2019

I'm going to merge even though coverage went down slightly.

@ljharb ljharb merged commit fcf2ce7 into import-js:master Jan 19, 2019
@ljharb ljharb deleted the travis branch January 19, 2019 18:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants