-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
fix a bunch of tests that were failing #1295
Conversation
a477df1
to
193d786
Compare
I'm a bit confused; there are two tests in that part of the file - one with I think we need to keep both tests, but it's entirely possible that they need tweaking to test the proper thing. (the rest of these changes look good) |
Sorry. Here is the test I'm talking about: https://github.com/benmosher/eslint-plugin-import/blob/master/tests/src/core/importType.js#L96 |
If we need that test, I'm not sure what to change to make it work. If you pull out the |
confirmed that removing the |
@ljharb just pinging to see what I should do with that last test |
I'm not really certain. When |
I agree with the extent comment. But how can we assume it's internal if it can't be resolved by the resolver? |
Are you suggesting this test was never good to have in the first place? |
@ljharb i dont think the test is good. Honestly, it feels like if the resolver can't resolve the path, then it should be unknown. But I imagine that would have to be a breaking change. Just removing that one check from the isExternalPath breaks 88 tests. So I can't imagine that we could change it and fix all the breaking tests without having a breaking change on our hands. So I guess I am wondering if we should remove the one failing test (because we can't make it pass currently), merge this, merge my other pr and then maybe there is a way forward in making that use case make more sense? One issue, I guess, is we dont know how long that test has been broken. But it seems like it wouldn't be viable to make that work without upping to a new major version. And if you are going to do that, then that test case seems like it should return 'unknown' and not 'internal' anyway |
We definitely aren't going to do a major version at this time. Since removing that check breaks a bunch of tests, it's clearly needed. If we want to remove or change this pair of tests, that might be fine, but i'd like to have a really good understanding of why the tests existed in the first place (since presumably they passed at one time). |
Yeah. I mean, i can comment it out, remove it, or whatever we want. I am going to try to dig and see if I can figure out when it broke. |
@jeffshaver woof, ok. I wonder what that would have caused, since the only difference between |
@ljharb What do you think is the path forward? |
I mean the fastest thing is to revert that commit; but it’d be worth spending some time first to figure out if there’s a way to keep it, and why it caused a failure. |
Just didn't know if we were going to try and put out a release before that. That commit is almost a year old. So that test will have been broken for quite some time. If we want to take the time and figure it out, than I understand that. Obviously I have a bit of bias with wanting this and my other PR to get in, so I can completely get off of TSLint on my project at work. Should I try to revert that commit and see if tests break or whatever? |
It might not be the commit, but it might be the latest version of resolve. What happens if you peg it at |
@ljharb tried pinning 1.9.0 and 1.6.0. Didn't work :( |
@jeffshaver then that implies that that commit isn't what broke tests, given that the tests passed on #1069 - but rather, that something else in our dependency graph got updated around that time, and started breaking things. |
@ljharb, maybe I'll downgrade the deps one by one to see if I can figure it out. I tried a few |
Note that it also might be a transitive dep, that you can't touch via package.json. You may want to try temporarily including a lockfile, using npm 6.9's |
@ljharb if we can pin the following packages and their transitive deps, then the test passes (ALL tests!): eslint-import-resolver-node@^0.3.2 |
I am having issues on how to get those all pinned (with a lockfile...). If i do an npm install and then an npm install --only=prod --before 04/06/2018, then all pass. but then a fresh npm install (from what should be the lockfile), it fails... |
ok - so let's try |
difference between the 6th and before the 8th show these differences:
|
When I run |
k, narrowed it down: a before date of 2019-02-05 passes, 2019-02-06 fails. A diff of the package-locks shows: $ diff 5.json 6.json
968,972d967
< "builtin-modules": {
< "version": "1.1.1",
< "resolved": "https://registry.npmjs.org/builtin-modules/-/builtin-modules-1.1.1.tgz",
< "integrity": "sha1-Jw8HbFpywC9bZaR9+Uxf46J4iS8="
< },
3234,3241d3228
< "is-builtin-module": {
< "version": "1.0.0",
< "resolved": "https://registry.npmjs.org/is-builtin-module/-/is-builtin-module-1.0.0.tgz",
< "integrity": "sha1-VAVy0096wxGfj3bDDLwbHgN6/74=",
< "requires": {
< "builtin-modules": "^1.0.0"
< }
< },
4190,4192c4177,4179
< "version": "2.4.2",
< "resolved": "https://registry.npmjs.org/normalize-package-data/-/normalize-package-data-2.4.2.tgz",
< "integrity": "sha512-YcMnjqeoUckXTPKZSAsPjUPLxH85XotbpqK3w4RyCwdFQSU5FxxBys8buehkSfg0j9fKvV1hn7O0+8reEgkAiw==",
---
> "version": "2.5.0",
> "resolved": "https://registry.npmjs.org/normalize-package-data/-/normalize-package-data-2.5.0.tgz",
> "integrity": "sha512-/5CMN3T0R4XTj4DcGaexo+roZSdSFW/0AOOTROrjxzCG1wrWXEsGbRKevjlIL+ZDE4sZlJr5ED4YW0yqmkK+eA==",
4195c4182
< "is-builtin-module": "^1.0.0",
---
> "resolve": "^1.10.0",
8737,8739c8724,8726
< "version": "3.7.0",
< "resolved": "https://registry.npmjs.org/tsconfig-paths/-/tsconfig-paths-3.7.0.tgz",
< "integrity": "sha512-7iE+Q/2E1lgvxD+c0Ot+GFFmgmfIjt/zCayyruXkXQ84BLT85gHXy0WSoQSiuFX9+d+keE/jiON7notV74ZY+A==",
---
> "version": "3.8.0",
> "resolved": "https://registry.npmjs.org/tsconfig-paths/-/tsconfig-paths-3.8.0.tgz",
> "integrity": "sha512-zZEYFo4sjORK8W58ENkRn9s+HmQFkkwydDG7My5s/fnfr2YYCaiyXe/HBUcIgU8epEKOXwiahOO+KZYjiXlWyQ==", Looks like it's normalize-package-date v2.5.0, but that suggests the test is badly written (because the only change there should be using resolve instead of is-builtin-module, which actually works properly). I'll look into it. |
…n erstwhile transitive dep
ha, turns out the tests were depending on |
@ljharb I went took a look at the tests that were failing. seemed like all but one of them existed because there were exports for things that didn't exist, i.e.
export { bar}
, whenbar
isn't defined.There is one test currently failing. However, after reading it, I think it should be deleted because what it says isn't true. The test reads:
should return 'internal' for module from 'node_modules' if 'node_modules' missed in 'external-module-folders'. However, the check for isExternalPath checks whether or not the
path` argument is falsy. If it is falsy, or if it is found in the import/external-module-folders, then it returns true. In the case of that test, the path is undefined (because that file doesn't exist), and therefore the test is failing.