-
-
Notifications
You must be signed in to change notification settings - Fork 116
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
#1124 Reason explanation id ambiguity #1125
#1124 Reason explanation id ambiguity #1125
Conversation
Would you be willing to write a test that specifically exercises this new code? |
For unit testing it looks trivial, but perhaps you mean another type of test. This is my first submission here, so I'm not quite familiar with test approach in this project - please give a good reference what you mean :) |
Well, I see failed test and probably functionalTest |
I think you could update |
@autonomousapps can you please recheck the PR? |
src/functionalTest/groovy/com/autonomousapps/jvm/BundleSpec.groovy
Outdated
Show resolved
Hide resolved
src/functionalTest/groovy/com/autonomousapps/jvm/BundleSpec.groovy
Outdated
Show resolved
Hide resolved
I've discovered one more related problem. The project submodules are represented with both first and second coordinates key segment. Consider sample module with dependencies that have keys in
and the filter Old behavior: it will take first non-ordered, there is ambiguity. In my experiment it took
The new behavior:
Instead of ambiguity it throws failure. But the error message can be confusing: we were filtering by WDYT? |
Do you still have concerns or just need more time for better validation of the change? |
Just more time. I've been underwater at work. Thanks for your patience. |
@@ -253,6 +253,29 @@ abstract class ReasonTask @Inject constructor( | |||
} | |||
|
|||
private fun wasFiltered(): Boolean = finalAdvice == null && unfilteredAdvice != null | |||
|
|||
internal companion object { | |||
internal fun findFilteredDependencyKey(dependencies: Set<Map.Entry<String, Any>>, requestedId: String): String? { |
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.
does this need to be internal instead of private?
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.
because of accessing from the unit test
import org.junit.jupiter.api.Assertions.assertThrows | ||
import org.junit.jupiter.api.Test | ||
|
||
class ReasonTaskTest { |
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.
Thank you for this great unit test
The solution proposal for #1124
Sample failure message:
and at the same time it works if
--id
is specified as:list
as it fully matches one of the dependencies full name.Sample test cases explaining updated behavior. Consider a project with these project dependencies:
:list
:list:sublist
:list-default
:list-impl
and library dependencies
com.squareup.okio:okio-jvm:3.0.0
com.squareup.okio:okio:3.0.0
--id
:li
Coordinates ':li' matches more than 1 dependency [:list, :list:sublist, :list-default, :list-impl]
:list
You asked about the dependency ':list'
:list:
You asked about the dependency ':list:sublist'
:list-d
You asked about the dependency ':list-default'
com.squareup.okio:oki
Coordinates 'com.squareup.okio:oki' matches more than 1 dependency [com.squareup.okio:okio-jvm:3.0.0, com.squareup.okio:okio:3.0.0]
com.squareup.okio:okio
You asked about the dependency 'com.squareup.okio:okio:3.0.0
com.squareup.okio:okio:
You asked about the dependency 'com.squareup.okio:okio:3.0.0
com.squareup.okio:okio:3.0.0
You asked about the dependency 'com.squareup.okio:okio:3.0.0
com.squareup.okio:okio-
You asked about the dependency 'com.squareup.okio:okio-jvm:3.0.0