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

Remove src deps from test deps on --pedantic-packages report #1092

Merged
merged 13 commits into from
Nov 6, 2023

Conversation

JordanMartinez
Copy link
Contributor

Description of the change

Fixes #1043

In checkImports, moved the main code into a where clause so that I could use the source project's used 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:

  • Added the change to the "Unreleased" section of the changelog
  • Added some example of the new feature to the README
  • Added a test for the contribution (if applicable)

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 😊

test/Spago/Build.purs Outdated Show resolved Hide resolved
Copy link
Member

@f-f f-f left a 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


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")
Copy link
Member

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

Copy link
Contributor Author

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.

Copy link
Member

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

Copy link
Contributor Author

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?

Copy link
Member

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

Copy link
Member

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

Copy link
Contributor Author

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 Show resolved Hide resolved
test/Spago/Build.purs Outdated Show resolved Hide resolved
Comment on lines 201 to 204
Spec.it "package config has 'pedantic_packages: true'" \{ spago, fixture } -> do
setupSrcUnusedDeps spago false
addPedanticPackagesToSrc
spago [ "build" ] >>= shouldBeFailureErr (fixture "check-unused-dependency.txt")
Copy link
Member

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 and effect
  • ...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.

Copy link
Contributor Author

@JordanMartinez JordanMartinez Oct 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The options forward are:

  1. Stop making test depend on all the packages declared by the source package implicitly. Rather, make it explicit. A test package only implicitly depends on its source package; that's it. While this simplifies this particular situation, it complicates spago install and spago uninstall situations. But hey, isn't that what the --pedantic-packages=autofix idea is for?
  2. 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 a spago uninstall can make a future spago build not work as (correct me if I'm wrong) spago build -p foo build both the source and test packages of foo.
  3. 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.

Copy link
Contributor Author

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?

Copy link
Member

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

Copy link
Member

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.

Copy link
Member

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?

test/Spago/Build.purs Outdated Show resolved Hide resolved
@f-f
Copy link
Member

f-f commented Oct 31, 2023

@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 setup function (this might be something that we could reuse for other tests too, but today's not the day for that)
I also moved all the fixtures to their own folders so it's easier to track them down.

There's one failing test now (together with a TODO in the code), for a case that I think we should catch, related to the source-and-test-dependencies interplay that we discussed above

@JordanMartinez
Copy link
Contributor Author

Hoping to get to this sometime this week, but next week might be more likely.

@JordanMartinez
Copy link
Contributor Author

There's one failing test now (together with a TODO in the code), for a case that I think we should catch, related to the source-and-test-dependencies interplay that we discussed above

Could you clarify what the issue here is? It's been a while.

@f-f
Copy link
Member

f-f commented Nov 6, 2023

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..

@f-f f-f merged commit 2f272e6 into master Nov 6, 2023
3 checks passed
@f-f f-f deleted the jam/fix-test-deps-output branch November 6, 2023 15:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

For pedantic builds, limit install recommendations for test deps
2 participants