Issue159/full exclude excluded modules #163
Conversation
|
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 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 |
# Conflicts: # README.md # affectedmoduledetector/src/main/kotlin/com/dropbox/affectedmoduledetector/AffectedModuleDetectorPlugin.kt # affectedmoduledetector/src/test/kotlin/com/dropbox/affectedmoduledetector/AffectedModuleDetectorPluginTest.kt # gradle.properties # sample/buildSrc/build.gradle.kts
Codecov Report
@@ 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
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
| val shouldInclude = (isRootProject || (isProjectAffected && isProjectProvided)) && isNotModuleExcluded | ||
| logger?.info("checking whether I should include ${project.path} and my answer is $shouldInclude") | ||
|
|
||
| return shouldInclude |
There was a problem hiding this comment.
I really had problems understanding the previous variant. I recommend using additional variables with understandable naming.
|
|
||
| @JvmStatic | ||
| fun getRootConfiguration(project: Project): AffectedModuleConfiguration { | ||
| if (affectedModuleConfiguration == null) { |
There was a problem hiding this comment.
rootConfiguration getting was called every time when "shouldInclude" fun was called. Hence, I transformed it to the singletone.
There was a problem hiding this comment.
As I posted in the other channel, getting the configuration is a map so there is no runtime benefit to this
| } | ||
|
|
||
| @JvmStatic | ||
| fun getTestConfiguration(project: Project): AffectedTestConfiguration { |
There was a problem hiding this comment.
getting of root configuration and test configuration should be in one place with one contract. It should be the same.
Now u can get any configuration from AMD file and it is clear for understanding and DRY also
There was a problem hiding this comment.
AffectedTestConfiguration only is used internally in the plugin so should exist inside the plugin class. There is no real benefit to exposing this as I'm not sure why another plugin would ever want to call this. Making it public and static means this could get called by another plugin without having initialized the plugin which just fail since this is non-nullable. Can you provide a usecase for getting this value outside the plugin? (tests don't count as we could just as easily have this helper function internal in the tests if we needed it)
Also, there is very little usecase for getting the configuration as well which is why its only called once as well before this change. You can get what you need from the instance of the AffectedModuleDetector in most cases
| val affectedPath = getAffectedPath(testType, project) | ||
| val configuration = AffectedModuleDetector.getRootConfiguration(project) | ||
| if (affectedPath == null || AffectedModuleDetector.isModuleExcluded(configuration, project)) { | ||
| return |
There was a problem hiding this comment.
The main line of this fix. We can skip calling .dependsOn and .afterEvaluate for excluded modules at all. I hope it will fix the problem of our colleague from Peru.
Because I guess, if the module has not a Detekt but one was called with .dependsOn, Detekt might try calling any deps, try to configure smth, and create a crash per issue 159.
|
@joshafeinberg could u explain me how can I generate and find failed codecov/patch report? Report build/reports/jacoco/testCoverage/html/index.html is not contain failed step data. |
| var rootProject = project | ||
| while (!rootProject.isRoot || rootProject.parent != null) { | ||
| rootProject = rootProject.parent!! | ||
| } |
There was a problem hiding this comment.
This while loop is unnecessary. project has a getRootProject() in it that is more efficient (rootProject is stored as a variable on the class so no need for a while loop
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 This is what I meant by my earlier comment that you didn't address
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 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 |
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
withPluginmethod for excluded modules at all. Because calltask.dependsOn(affectedPath)andproject.tasks.findByPath(affectedPath)?.onlyIffor excluded modules really unecessary.And I hope thes optimization help to solve this issue.
What I did?
withPluginfor excluded modules.AffectedModuleConfigurationandAffectedTestConfigurationusing simple singletone pattern.Explanation:
'- We can to know that module is excluded earlier via
isModuleExcluded'- Optimization of
shouldIncludemethod, 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?