fix failing tests on master #15
Conversation
graingert
force-pushed
the
fix-failing-tests-on-master
branch
from
b3006f0 to
5cf14d9
Compare
Codecov Report
@@ Coverage Diff @@
## master #15 +/- ##
==========================================
- Coverage 97.52% 97.47% -0.05%
==========================================
Files 18 18
Lines 363 357 -6
==========================================
- Hits 354 348 -6
Misses 9 9
Continue to review full report at Codecov.
|
graingert
force-pushed
the
fix-failing-tests-on-master
branch
from
5cf14d9 to
1d58ab5
Compare
|
hey, @mysticatea your tests are failing on master! |
mysticatea
left a comment
mysticatea
left a comment
There was a problem hiding this comment.
Thank you for contributing.
Is my understanding correct that this PR includes the following things:
- Fixing some tests.
- Updating dependencies.
- Addin Node 8 to CI.
Plus, I'd not like to include package-lock.json. Could you remove it?
.travis.yml
Outdated
| after_success: | ||
| - npm run codecov | ||
| - "8" | ||
| before_install: npm install -g npm@latest |
There was a problem hiding this comment.
Yep, so that npm@5 or more is installed (only the latest major version is supported)
There was a problem hiding this comment.
The requirements of this package include Node.js ^4.0.0 1. I think this implies this package expects to work with the npm that Node.js 4.0.0 bundles. If the upgrade of npm is needed, I cannot merge this PR.
There was a problem hiding this comment.
I guess the other option is yarn. Without a lock file of some sort a problem like this will reoccur
There was a problem hiding this comment.
The root cause of the problem is that tests included invalid syntax. The failing is natural. I think I should run CI regularly and fix problems.
There was a problem hiding this comment.
You can configure that with Travis Cron beta
There was a problem hiding this comment.
Thank you! I will configure it later.
There was a problem hiding this comment.
The other option is for a master only set of tests that delete the package lock before install. This way PRs are not effected but you still get to know about bugs
package-lock.json
Outdated
| @@ -0,0 +1,4625 @@ | |||
| { | |||
There was a problem hiding this comment.
Could you add .npmrc with package-lock=false and remove this package-lock.json?
See also: vuejs/eslint-plugin-vue#128
There was a problem hiding this comment.
Package lock is included to prevent master from failing without any changes
There was a problem hiding this comment.
I want not to merge package-lock.json into this repo because it damages the value of CI builds, the file disallows CI builds running tests with dependencies that users install actually.
| {code: "import a from \"a\"; var a;", parserOptions: {sourceType: "module"}, errors: [{type: "Identifier", message: "\"a\" is already defined."}]}, | ||
| {code: "import a from \"a\"; import a from \"b/a\";", parserOptions: {sourceType: "module"}, errors: [{type: "Identifier", message: "\"a\" is already defined."}]}, | ||
| {code: "{ var a; { var a; } }", errors: [{type: "Identifier", message: "\"a\" is already defined in the upper scope."}]}, | ||
| {code: "import a from \"a\"; { var a; }", parserOptions: {sourceType: "module"}, errors: [{type: "Identifier", message: "\"a\" is already defined in the upper scope."}]}, |
There was a problem hiding this comment.
Just a question, do 3 test cases which include import declarations throw syntax error currently?
There was a problem hiding this comment.
Yes. You can't shadow an import at all in modern babel
graingert
force-pushed
the
fix-failing-tests-on-master
branch
from
d350dd3 to
768155b
Compare
Alright, but it means that PRs will fail unnecessarily |
|
@mysticatea looks like node 4 needs the new version of npm. |
|
Could you remove cache of |
graingert
force-pushed
the
fix-failing-tests-on-master
branch
from
768155b to
28156ba
Compare
|
@mysticatea removed cache. |
|
@mysticatea oh yeah, that did it. |
3 participants
No description provided.