-
Notifications
You must be signed in to change notification settings - Fork 149
add test case for not having test.resourceDir in main scope #3879
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
base: main
Are you sure you want to change the base?
add test case for not having test.resourceDir in main scope #3879
Conversation
The directive is declared here: scala-cli/modules/directives/src/main/scala/scala/build/preprocessing/directives/Resources.scala Lines 34 to 36 in 418959f
and then put into options, here: scala-cli/modules/directives/src/main/scala/scala/build/preprocessing/directives/Resources.scala Line 40 in 418959f
At a glance, test scope is being passed as a requirement... |
We construct the build options with the resource directory here: scala-cli/modules/directives/src/main/scala/scala/build/preprocessing/directives/Resources.scala Lines 46 to 70 in 418959f
It seems it's then extracted while processing sources here: scala-cli/modules/build/src/main/scala/scala/build/CrossSources.scala Lines 398 to 412 in 418959f
To debug this, I'd track if build requirements (being the test scope) is respected when this field is being passed around:
Let me know if this helps, this is where I'd start. |
(note: I converted the PR to a draft, since it's not ready for review) |
74b7f89
to
ce1a868
Compare
expect(resourcesDirs.exists(_.last == "mainResources")) | ||
val hasTestResources = resourcesDirs.exists(_.last == "testResources") | ||
expect(if isTestScope then hasTestResources else !hasTestResources) | ||
expect(resourcesDirs.toSet == sourcesResourcesDirs.toSet) |
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 not sure if these tests were testing the resolving of test scope resourceDir; it was testing only if the build options are passed correctly.
I was expecting that this would be equal, but it's not. It wasn't even before my changes. Now this test is failing.
.filter(!resourceDirsFromCli.contains(_)) | ||
.map(ResourceDirectory(_)) | ||
} | ||
val finalInputs = allInputs.add(resourceDirectoriesFromDirectives) |
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.
Removing this resolves my issue and fixes the tests. The resourceDirectoriesFromDirectives
extracts the directives and puts them in the input without scope. Later on in the resolveResourceDirs, the paths were extracted from input and wrapped into WithBuildRequirements
without scope (because it wasn't available at this point) using the default main
scope.
However, I'm not sure what the other consequences of this change are, as the allInputs
are returned from the forInputs
method. I don't know where this is used yet.
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.
right... so I'm guessing we need to respect the scope 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.
Another option to address the issue will be to pass the allInputs
to resoveReourceDirs
instead of finalInputs
I found that the resource from the directive
test.resourceDir
is added to the classpath in the main scope. It should be only in the test scope.I thought that was an easy fix, so I tried to do so. However, I failed. I'm opening a PR with a test case for this. I'm happy to fix it if you could give me some guidance on where to look.