dropbox
Issue159/full exclude excluded modules
Issue159/full exclude excluded modules
I tyied to solve this issue. We can see crashes on the excluded modules.
I couldn't to reproduce this issue and logic of excluded modules works correctly.
I couldn't to reproduce this issue and logic of excluded modules works correctly. But I suppose it might depend from configuration each custom task.
I gess that the key problem available because we call task.dependsOn(affectedPath) and call other gradle logic on each module doesn't depends on the excluded or included one.
Hence I thought that we can except call of extra code from withPlugin method for excluded modules at all. Because call task.dependsOn(affectedPath) and project.tasks.findByPath(affectedPath)?.onlyIf for excluded modules really unecessary.
And I hope thes optimization help to solve this issue.
What I did?
- Exepted calling extra code from
withPluginfor excluded modules. - Refactored getting
AffectedModuleConfigurationandAffectedTestConfigurationusing simple singletone pattern. Explanation: '- We can to know that module is excluded earlier viaisModuleExcluded'- Optimization ofshouldIncludemethod, because it called really quiqly and we can use cache of singletone now '- Getting of any configuration is the same approuch.
What do they think about this?
I haven't been able to reproduce this issue either but could you please explain more about how this is the fix?
The problem as far as I can see is that we don't know the excluded modules until after gradle configuration whereas the task.dependsOn call happens during configuration so we need to find a way to pass those excluded modules through to handle this better
The only things I see in this PR are a lot of style changes that don't actually address the issue. It also seems to break some tests and introduces some functionality that shouldn't change (from what I can see in AffectedModuleDetectorPluginTask tests now fail without having an AffectedTestConfiguration which should be done automatically here https://github.com/dropbox/AffectedModuleDetector/blob/main/affectedmoduledetector/src/main/kotlin/com/dropbox/affectedmoduledetector/AffectedModuleDetectorPlugin.kt#L70)
Codecov Report
Merging #163 (afb7b33) into main (f041be2) will increase coverage by
1.94%. The diff coverage is41.86%.
@@ Coverage Diff @@
## main #163 +/- ##
============================================
+ Coverage 50.37% 52.31% +1.94%
- Complexity 66 68 +2
============================================
Files 13 13
Lines 532 539 +7
Branches 99 100 +1
============================================
+ Hits 268 282 +14
+ Misses 237 224 -13
- Partials 27 33 +6
| Impacted Files | Coverage Δ | |
|---|---|---|
| ...ctedmoduledetector/AffectedModuleDetectorPlugin.kt | 56.56% <20.00%> (+5.56%) |
:arrow_up: |
| ...x/affectedmoduledetector/AffectedModuleDetector.kt | 45.10% <60.86%> (+3.06%) |
:arrow_up: |
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.
![codecov[bot] avatar](https://avatars.githubusercontent.com/in/254?v=4 "Published by a pub.dev verified publisher"){kind=small}
@joshafeinberg could u explain me how can I generate and find failed codecov/patch report?
I see, that in ci_test_and_publish.yml using the command ./gradlew assemble testCoverage for the generation report. But I can't find any local report which shows 41.86% vs the target of 50.37%.
Report build/reports/jacoco/testCoverage/html/index.html is not contain failed step data.
@joshafeinberg could u explain me how can I generate and find failed codecov/patch report? I see, that in
ci_test_and_publish.ymlusing the command./gradlew assemble testCoveragefor the generation report. But I can't find any local report which shows 41.86% vs the target of 50.37%.Report build/reports/jacoco/testCoverage/html/index.html is not contain failed step data.
This is handled by the codecov bot so I don't know of an easy way to do this locally
Running this with the test I created here (https://github.com/dropbox/AffectedModuleDetector/pull/167/files#diff-cb9fdd7643cbfaee5b6e4075c1fbfffa256e3984f7148218696c1a451f719617) which actually runs using the gradle runner. You can see if I exclude sample-core in the logs I still get many instances of tasks on sample-core running with your version
value of:
getOutput()
expected not to contain:
:sample-core:assembleAndroidTest SKIPPED
but was:
Warning: Mapping new ns http://schemas.android.com/repository/android/common/02 to old ns http://schemas.android.com/repository/android/common/01
Warning: Mapping new ns http://schemas.android.com/repository/android/generic/02 to old ns http://schemas.android.com/repository/android/generic/01
Warning: Mapping new ns http://schemas.android.com/sdk/android/repo/addon2/02 to old ns http://schemas.android.com/sdk/android/repo/addon2/01
Warning: Mapping new ns http://schemas.android.com/sdk/android/repo/repository2/02 to old ns http://schemas.android.com/sdk/android/repo/repository2/01
Warning: Mapping new ns http://schemas.android.com/sdk/android/repo/sys-img2/02 to old ns http://schemas.android.com/sdk/android/repo/sys-img2/01
Warning: Mapping new ns http://schemas.android.com/repository/android/common/02 to old ns http://schemas.android.com/repository/android/common/01
Warning: Mapping new ns http://schemas.android.com/repository/android/generic/02 to old ns http://schemas.android.com/repository/android/generic/01
Warning: Mapping new ns http://schemas.android.com/sdk/android/repo/addon2/02 to old ns http://schemas.android.com/sdk/android/repo/addon2/01
Warning: Mapping new ns http://schemas.android.com/sdk/android/repo/repository2/02 to old ns http://schemas.android.com/sdk/android/repo/repository2/01
Warning: Mapping new ns http://schemas.android.com/sdk/android/repo/sys-img2/02 to old ns http://schemas.android.com/sdk/android/repo/sys-img2/01
:sample-app:preBuild SKIPPED
:sample-app:preDebugBuild SKIPPED
:sample-core:preBuild SKIPPED
:sample-core:preDebugBuild SKIPPED
:sample-core:compileDebugAidl SKIPPED
:sample-app:compileDebugAidl SKIPPED
:sample-core:packageDebugRenderscript SKIPPED
:sample-app:compileDebugRenderscript SKIPPED
:sample-app:generateDebugBuildConfig SKIPPED
:sample-core:writeDebugAarMetadata SKIPPED
:sample-app:checkDebugAarMetadata SKIPPED
:sample-app:generateDebugResValues SKIPPED
:sample-app:generateDebugResources SKIPPED
:sample-core:compileDebugRenderscript SKIPPED
:sample-core:generateDebugResValues SKIPPED
:sample-core:generateDebugResources SKIPPED
:sample-core:packageDebugResources SKIPPED
:sample-app:mergeDebugResources SKIPPED
:sample-app:createDebugCompatibleScreenManifests SKIPPED
:sample-app:extractDeepLinksDebug SKIPPED
:sample-core:extractDeepLinksDebug SKIPPED
:sample-core:processDebugManifest SKIPPED
:sample-app:processDebugMainManifest SKIPPED
:sample-app:processDebugManifest SKIPPED
:sample-app:processDebugManifestForPackage SKIPPED
:sample-core:compileDebugLibraryResources SKIPPED
:sample-core:parseDebugLocalResources SKIPPED
:sample-core:generateDebugRFile SKIPPED
:sample-app:processDebugResources SKIPPED
:sample-core:generateDebugBuildConfig SKIPPED
:sample-core:compileDebugKotlin SKIPPED
:sample-core:javaPreCompileDebug SKIPPED
:sample-core:compileDebugJavaWithJavac SKIPPED
:sample-core:bundleLibCompileToJarDebug SKIPPED
:sample-app:compileDebugKotlin SKIPPED
:sample-app:javaPreCompileDebug SKIPPED
:sample-app:compileDebugJavaWithJavac SKIPPED
:sample-app:bundleDebugClasses SKIPPED
:sample-app:preDebugAndroidTestBuild SKIPPED
:sample-app:compileDebugAndroidTestAidl SKIPPED
:sample-app:processDebugAndroidTestManifest SKIPPED
:sample-app:compileDebugAndroidTestRenderscript SKIPPED
:sample-app:generateDebugAndroidTestBuildConfig SKIPPED
:sample-app:checkDebugAndroidTestAarMetadata SKIPPED
:sample-app:generateDebugAndroidTestResValues SKIPPED
:sample-app:generateDebugAndroidTestResources SKIPPED
:sample-app:mergeDebugAndroidTestResources SKIPPED
:sample-app:processDebugAndroidTestResources SKIPPED
:sample-app:compileDebugAndroidTestKotlin SKIPPED
:sample-app:javaPreCompileDebugAndroidTest SKIPPED
:sample-app:compileDebugAndroidTestJavaWithJavac SKIPPED
:sample-app:compileDebugAndroidTestSources SKIPPED
:sample-app:mergeDebugAndroidTestShaders SKIPPED
:sample-app:compileDebugAndroidTestShaders SKIPPED
:sample-app:generateDebugAndroidTestAssets SKIPPED
:sample-core:mergeDebugShaders SKIPPED
:sample-core:compileDebugShaders SKIPPED
:sample-core:generateDebugAssets SKIPPED
:sample-core:packageDebugAssets SKIPPED
:sample-app:mergeDebugAndroidTestAssets SKIPPED
:sample-app:compressDebugAndroidTestAssets SKIPPED
:sample-app:processDebugAndroidTestJavaRes SKIPPED
:sample-core:processDebugJavaRes SKIPPED
:sample-core:bundleLibResDebug SKIPPED
:sample-app:mergeDebugAndroidTestJavaResource SKIPPED
:sample-app:mergeDebugAndroidTestJniLibFolders SKIPPED
:sample-core:mergeDebugJniLibFolders SKIPPED
:sample-core:mergeDebugNativeLibs SKIPPED
:sample-core:stripDebugDebugSymbols SKIPPED
:sample-core:copyDebugJniLibsProjectOnly SKIPPED
:sample-app:mergeDebugAndroidTestNativeLibs SKIPPED
:sample-core:bundleLibRuntimeToJarDebug SKIPPED
:sample-app:checkDebugAndroidTestDuplicateClasses SKIPPED
:sample-app:dexBuilderDebugAndroidTest SKIPPED
:sample-app:mergeExtDexDebugAndroidTest SKIPPED
:sample-app:mergeDexDebugAndroidTest SKIPPED
:sample-app:validateSigningDebugAndroidTest SKIPPED
:sample-app:packageDebugAndroidTest SKIPPED
:sample-app:assembleDebugAndroidTest SKIPPED
:sample-core:preDebugAndroidTestBuild SKIPPED
:sample-core:compileDebugAndroidTestAidl SKIPPED
:sample-core:processDebugAndroidTestManifest SKIPPED
:sample-core:compileDebugAndroidTestRenderscript SKIPPED
:sample-core:generateDebugAndroidTestBuildConfig SKIPPED
:sample-core:checkDebugAndroidTestAarMetadata SKIPPED
:sample-core:generateDebugAndroidTestResValues SKIPPED
:sample-core:generateDebugAndroidTestResources SKIPPED
:sample-core:mergeDebugAndroidTestResources SKIPPED
:sample-core:processDebugAndroidTestResources SKIPPED
:sample-core:compileDebugAndroidTestKotlin SKIPPED
:sample-core:javaPreCompileDebugAndroidTest SKIPPED
:sample-core:compileDebugAndroidTestJavaWithJavac SKIPPED
:sample-core:compileDebugAndroidTestSources SKIPPED
:sample-core:mergeDebugAndroidTestShaders SKIPPED
:sample-core:compileDebugAndroidTestShaders SKIPPED
:sample-core:generateDebugAndroidTestAssets SKIPPED
:sample-core:mergeDebugAndroidTestAssets SKIPPED
:sample-core:compressDebugAndroidTestAssets SKIPPED
:sample-core:processDebugAndroidTestJavaRes SKIPPED
:sample-core:mergeDebugAndroidTestJavaResource SKIPPED
:sample-core:mergeDebugAndroidTestJniLibFolders SKIPPED
:sample-core:mergeDebugAndroidTestNativeLibs SKIPPED
:sample-core:checkDebugAndroidTestDuplicateClasses SKIPPED
:sample-core:dexBuilderDebugAndroidTest SKIPPED
:sample-core:mergeExtDexDebugAndroidTest SKIPPED
:sample-core:mergeDexDebugAndroidTest SKIPPED
:sample-core:validateSigningDebugAndroidTest SKIPPED
:sample-core:packageDebugAndroidTest SKIPPED
:sample-core:assembleDebugAndroidTest SKIPPED
:sample-core:assembleAndroidTest SKIPPED
:assembleAffectedAndroidTests SKIPPED
This is what I meant by my earlier comment that you didn't address
The problem as far as I can see is that we don't know the excluded modules until after gradle configuration whereas the task.dependsOn call happens during configuration so we need to find a way to pass those excluded modules through to handle this better
It's really important to be cognizant of the gradle lifecycle and thats why stuff like just applying unit tests doesn't always work perfect especially if you can't reproduce the issue
You also never address my point about how in the tests you needed to manually add the AffectedTestConfiguration (https://github.com/dropbox/AffectedModuleDetector/pull/163/files#diff-3d7279e230fc30c188dcc589256cf41f5a263c752ebf7689a718ae6e920ba9f6) when I pointed out that this is done automatically (https://github.com/dropbox/AffectedModuleDetector/blob/main/affectedmoduledetector/src/main/kotlin/com/dropbox/affectedmoduledetector/AffectedModuleDetectorPlugin.kt#L70)
I believe my fix makes sense with a slight reordering of the function calls solves all the problems you brought up and provides a simpler solution (my PR outside the tests is +15/-5) so unless you want to fix up all the issues I brought up here we can just close this PR