-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Separate out project dependencies, add integration test to verify #1785
Conversation
There files are added accidentally, aren't they? (Note |
oh right yeah. I'll clean that up. I'm mostly curious what people think of this test. |
How about these?
|
What about a task |
I actually prefer (1). It doesn't look crazy, we already have the code, and it doesn't add any new production code. To me, |
Also running tasks/goals in tests is a pretty big slow down, which can be annoying. |
Well I still need to load the project context which is what I'm explicitly testing here. I can trust (from gradle) that JavaContainerBuilder is doing the right thing. What I need to know is that the gradle plugin code is doing the right things splitting the dependencies. |
2f5a8ff
to
158fa93
Compare
Pushed a new less ridiculous test. |
jib-gradle-plugin/src/test/resources/gradle/projects/all-local-multi-service/build.gradle
Outdated
Show resolved
Hide resolved
...ion-test/java/com/google/cloud/tools/jib/gradle/GradleLayerConfigurationIntegrationTest.java
Show resolved
Hide resolved
...ion-test/java/com/google/cloud/tools/jib/gradle/GradleLayerConfigurationIntegrationTest.java
Show resolved
Hide resolved
13cf140
to
9abda1a
Compare
...ion-test/java/com/google/cloud/tools/jib/gradle/GradleLayerConfigurationIntegrationTest.java
Outdated
Show resolved
Hide resolved
artifact -> | ||
artifact.getId().getComponentIdentifier() | ||
instanceof ProjectComponentIdentifier) | ||
.map(ResolvedArtifact::getFile) |
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.
This is unclear, but should we do filter(File::exists)
? For what I know, if do the filtering for allFiles
and classesOutputDirectories
. (JavaContainerBuilder
does the check and throws NoSuchFileException
, which might be a good thing actually. It's unclear to me if this can ever happen.)
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 it's safer to not filter, if we have an issue in the configuration, it should pick it up. File::exists
will just quietly ignore the error.
...est/resources/gradle/projects/all-local-multi-service/lib/src/test/java/com/lib/LibTest.java
Outdated
Show resolved
Hide resolved
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.
This looks good. I think the test worked out nicely.
Uh so here's the PR with the weirdo test. Basically injecting a task to inspect the layer configuration state and outputting it so the test can pick it up and verify. I don't know how else to do it without mocking the world.