-
Notifications
You must be signed in to change notification settings - Fork 131
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
Remove src deps from test deps on --pedantic-packages
report
#1092
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't go through the implementation yet, just trying to make sense of the tests for now. There's something about how the tests are setup that adds indirection - not sure if we need more abstraction or less there, will sleep on it
test/Spago/Build.purs
Outdated
|
||
Spec.it "passed --pedantic-packages CLI flag" \{ spago, fixture } -> do | ||
setupSrcTransitiveTests spago true | ||
spago [ "build", "--pedantic-packages" ] >>= shouldBeFailureErr (fixture "check-direct-import-transitive-dependency.txt") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd expect this to complain about the test packages containing superfluous console
and effect
(since they are already in the source dependencies) - the flag should turn it on for both src and test targets, since it's a one-off, i.e. the user is explicitly asking for a cleanup pass
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It did. That's why the true
arg in setupSrcTransitiveTests spago true
installs those deps in test
. Otherwise, I'll get an error about that. In this particular group of tests, we're testing whether source packages alone get that error, which I can't do since --pedantic-packages
enables that check on the test
code as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think my confusion comes from the fact that it's hard to see what the test is trying to verify - so I suggest we:
- do not generate the outputs via code, but only use fixtures
- this means that we'll have a lot of fixtures, so let's stick them in a separate folder, like
test-fixtures/pedantic
- then before each test case let's have a few lines summarising why the test should return what it returns
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's... a lot of work. Is there an alternative for making these tests clearer such as comments?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can merge and I can do the work if you'd prefer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can also pile commits on this branch if you'd like a self-contained package
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with you pushing commits here.
test/Spago/Build.purs
Outdated
Spec.it "package config has 'pedantic_packages: true'" \{ spago, fixture } -> do | ||
setupSrcUnusedDeps spago false | ||
addPedanticPackagesToSrc | ||
spago [ "build" ] >>= shouldBeFailureErr (fixture "check-unused-dependency.txt") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we have another issue here:
- the source dependencies have
console
andeffect
- ...but the sources don't use them
- the test dependencies don't have them
- ...but the test sources do use them
We correctly complain about them being unused (because we have the flag set to true in the package), but since the flag is not set in tests we don't say anything there.
However removing them would make the build fail, since they are used in tests. Not sure how to disentangle this one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The options forward are:
- Stop making
test
depend on all the packages declared by the source package implicitly. Rather, make it explicit. Atest
package only implicitly depends on its source package; that's it. While this simplifies this particular situation, it complicatesspago install
andspago uninstall
situations. But hey, isn't that what the--pedantic-packages=autofix
idea is for? - Add a
purs graph
call that allows one to see which dependencies in the source package are actually used and have the test package only implicitly depend on those used dependencies. While this resolves this issue, it'll be confusing because aspago uninstall
can make a futurespago build
not work as (correct me if I'm wrong)spago build -p foo
build both the source and test packages offoo
. - Add another check for this situation. If source declares unused deps that tests does not declare but which uses, then include it as an error. Since we're already going to be reporting that the source package has unused dependencies and thereby failing the build, we may also state the above issue.
Out of these three, 3 is my preference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any new thoughts on this? Or are you still thinking about it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still thinking about the overall shape of this PR, but I think I also prefer option 3 here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think option 1 is a possibility but definitely uncomfortable, so I'd like to reach for it only in case we can't make anything else work.
As for Option 2, I am not sure how it resolves the issue - I think spago uninstall
is supposed to be able to break the build, so that part is fine. The test package already implicitly depends on the packages, the issue is that we're not detecting if we are slating these implicit dependencies for removal.
Option 3 makes sense, and I'd go even further in the source+test packages integration, and add another kind of "error" that we return - on top of "unused" and "undeclared transitive" - which is "unused in source but used in test" (I'm sure we can find a more compact naming 😄). So in the same graph check we'd take the unused packages for the source package, and see if they'd be used in the test package. If yes, we add spago install --test-deps
instructions for them, which should counterbalance the spago uninstall
ones.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tests are all passing so maybe we don't need any of this?
@JordanMartinez I pushed a commit with a refactor to the test setup - I now know that my confusion was due to the "scene setup" for each test, that was imperative and interleaved with the logic of the test - I made that declarative with the There's one failing test now (together with a |
Hoping to get to this sometime this week, but next week might be more likely. |
Could you clarify what the issue here is? It's been a while. |
Uh, it turns out the implementation is already doing what we think it should do and the test was failing for a bad fixture. I'll sort out the merge conflict and merge this in.. |
Description of the change
Fixes #1043
In
checkImports
, moved the main code into a where clause so that I could use the source project'sused
deps rather than their declared ones.Updated the various tests to pass while adding a new test for pedantic packages that's more exhaustive.
Checklist:
README
P.S.: the above checks are not compulsory to get a change merged, so you may skip them. However, taking care of them will result in less work for the maintainers and will be much appreciated 😊