-
Notifications
You must be signed in to change notification settings - Fork 310
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
Add functional tests for the GradleInspector #6882
Conversation
@@ -396,10 +396,10 @@ packages: | |||
description: "An HTTP+HTTP/2 client for Android and Java applications" | |||
homepage_url: "https://github.com/square/okhttp/okhttp" | |||
binary_artifact: | |||
url: "" | |||
url: "https://repo.maven.apache.org/maven2/com/squareup/okhttp3/okhttp/3.10.0/okhttp-3.10.0.jar" |
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 changes in this file are obviously to the better.
packages: [] | ||
issues: |
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 GI reports these on the project level instead of per-scope, which probably makes sense as repositories are also defined on the project level. This implies some nice de-duplication.
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 would say dependencies are defined on the scope level. What if the errors occur further down in the tree, would they also be reporter on the project level instead of inside the tree?
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 would say dependencies are defined on the scope level.
True. What I meant was: Repositories are defined on the project level, so errors caused by no repository being present could be regarded as project configuration errors, and in that sense it makes sense (heh) to report the errors on the project level. However, missing repos are probably not the only cause for a dependency to be unresolved.
Probably the question boils down to: Is the position in the dependency tree important, i.e. may it have any influence on the error. I guess not.
But I'm undecided. I'm fine either way, to record unresolved dependencies (deduplicated) on the project level, or inside the dependency tree (with possible duplicates).
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.
Probably the question boils down to: Is the position in the dependency tree important, i.e. may it have any influence on the error. I guess not.
But I'm undecided. I'm fine either way, to record unresolved dependencies (deduplicated) on the project level, or inside the dependency tree (with possible duplicates).
For direct dependencies this might not be so important, but for transitive dependencies it can be very helpful to see where in the tree the issue occurs. I would appreciate if we would report issues inside the tree where possible.
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.
but for transitive dependencies it can be very helpful to see where in the tree the issue occurs.
Just out of curiosity, could you give an example where knowing the position in the tree is more helpful than having the issue at the top?
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.
Looks like you're still fine for now with the current state as you've approved, so I'll eventually fix this up later to get the tests in now.
...-inspector/src/funTest/assets/projects/synthetic/gradle-expected-output-lib-without-repo.yml
Outdated
Show resolved
Hide resolved
@@ -24,14 +24,6 @@ project: | |||
dependencies: | |||
- id: "Maven:org.jetbrains:annotations:13.0" | |||
- id: "Maven:org.jetbrains.kotlin:kotlin-stdlib-common:1.7.20" | |||
- name: "implementationDependenciesMetadata" |
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.
GI generally skips "DependenciesMetadata" configurations / scopes.
@@ -161,14 +113,6 @@ project: | |||
dependencies: | |||
- id: "Maven:org.jetbrains:annotations:13.0" | |||
- id: "Maven:org.jetbrains.kotlin:kotlin-stdlib-common:1.7.20" | |||
- name: "testImplementationDependenciesMetadata" |
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.
GI generally skips "DependenciesMetadata" configurations / scopes.
@@ -22,12 +22,6 @@ project: | |||
dependencies: | |||
- id: "Maven:org.jetbrains:annotations:13.0" | |||
- id: "Maven:org.jetbrains.kotlin:kotlin-stdlib-common:1.7.20" | |||
- name: "implementationDependenciesMetadata" |
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.
GI generally skips "DependenciesMetadata" configurations / scopes.
@@ -148,12 +105,6 @@ project: | |||
dependencies: | |||
- id: "Maven:org.jetbrains:annotations:13.0" | |||
- id: "Maven:org.jetbrains.kotlin:kotlin-stdlib-common:1.7.20" | |||
- name: "testImplementationDependenciesMetadata" |
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.
GI generally skips "DependenciesMetadata" configurations / scopes.
@@ -42,9 +34,6 @@ project: | |||
- id: "Maven:org.jetbrains.kotlin:kotlin-reflect:1.7.20" | |||
dependencies: | |||
- id: "Maven:org.jetbrains.kotlin:kotlin-stdlib:1.7.20" | |||
dependencies: |
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.
kotlin-stdlib
's dependencies were already seen on a level closer to the root (see line 49 and below), so they are skipped here. I'm not sure why the Gradle
package manager implementation does not skip it, too. Similar for other cases below.
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 why the
Gradle
package manager implementation does not skip it, too.
That's because the "legacy" Gradle analyzer natively creates a graph which is then resolved back to a tree for test result comparison, and this expands the cycles again.
Codecov ReportPatch coverage has no change and project coverage change:
Additional details and impacted files@@ Coverage Diff @@
## main #6882 +/- ##
============================================
- Coverage 64.37% 64.35% -0.03%
Complexity 1976 1976
============================================
Files 331 331
Lines 16729 16744 +15
Branches 2380 2385 +5
============================================
+ Hits 10770 10775 +5
- Misses 4919 4929 +10
Partials 1040 1040
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
9b6a7a2
to
9df8df5
Compare
Note that I've extracted the required Android SDK upgrade to #6887. |
9df8df5
to
fbc791a
Compare
fbc791a
to
8ab1a3a
Compare
7e083e3
to
65547d9
Compare
Do not run the `funTest` tasks from the `gradle-inspector` and `gradle-package-manager` projects in parallel to avoid issues in `GradleAndroidFunTest` when dynamically installing missing Android SDK components, as both Gradle projects will start to share the same test projects in an upcoming change. Signed-off-by: Sebastian Schuberth <sschuberth@gmail.com>
57c8bd7
to
e431d15
Compare
@mnonnenmacher @fviernau this is up for review again as I've found the cause for the failing |
Copy functional tests and expected results (but not the test projects) from the `Gradle` package manager implementation and adjust tests so that they run, but not necessarily pass yet. A later commit will adjust the expected results to show the differences with the `GradleInspector` implementation. Signed-off-by: Sebastian Schuberth <sschuberth@gmail.com>
Signed-off-by: Sebastian Schuberth <sschuberth@gmail.com>
Update expected results from the `Gradle` package manager to match those for the `GradleInspector` package manager. This highlights the differences between the two implementations. Signed-off-by: Sebastian Schuberth <sschuberth@gmail.com>
Do not cut the dependency tree on any already visited dependency in a configuration, but only for those that appear on the path from the root to a leaf. This fixes the discrepancy compared to the `Gradle` package manager that was identified at [1] but justified wrongly. [1]: #6882 (comment) Signed-off-by: Sebastian Schuberth <sebastian@doubleopen.org>
Do not cut the dependency tree on any already visited dependency in a configuration, but only for those that appear on the path from the root to a leaf. This fixes the discrepancy compared to the `Gradle` package manager that was identified at [1] but justified wrongly. [1]: #6882 (comment) Signed-off-by: Sebastian Schuberth <sebastian@doubleopen.org>
Please have a look at the individual commit messages for the details.