-
Notifications
You must be signed in to change notification settings - Fork 155
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 project dependency mutation #368
Conversation
b66e2a3
to
70b4c2a
Compare
70b4c2a
to
b0baab1
Compare
75a69c1
to
5099b4b
Compare
5099b4b
to
a6371b3
Compare
src/main/java/org/gradle/profiler/mutations/support/FileSupport.java
Outdated
Show resolved
Hide resolved
src/main/java/org/gradle/profiler/mutations/support/FileSupport.java
Outdated
Show resolved
Hide resolved
src/main/java/org/gradle/profiler/mutations/FileChangeMutatorConfigurator.java
Outdated
Show resolved
Hide resolved
src/main/java/org/gradle/profiler/mutations/support/ProjectCombinations.java
Outdated
Show resolved
Hide resolved
src/main/java/org/gradle/profiler/mutations/support/ProjectCombinationsSupport.java
Outdated
Show resolved
Hide resolved
src/main/java/org/gradle/profiler/mutations/support/ProjectCombinationsSupport.java
Outdated
Show resolved
Hide resolved
...est/groovy/org/gradle/profiler/mutations/ApplyProjectDependencyChangeSetupMutatorTest.groovy
Outdated
Show resolved
Hide resolved
Co-authored-by: Lóránt Pintér <lorant@gradle.com>
d6e3c3d
to
586b0bb
Compare
…tor and ApplyProjectDependencyChangeSetupMutator
6ed99ea
to
5ac46f1
Compare
5ac46f1
to
b104d9a
Compare
import static com.google.common.base.Preconditions.checkArgument; | ||
import static org.gradle.profiler.mutations.ApplyProjectDependencyChangeMutatorConfigurator.APPLIED_PROJECTS_COUNT_KEY; | ||
|
||
public class ProjectCombinationsSupport { |
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.
Why not have these methods on ProjectCombinations
?
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 really like "data only classes". So classes that don't have any logic in there (except maybe trivial creator methods), since that makes other code easier to test. Also having such code in classes then you soon end with multiple similar static methods and class gets bloated and harder to read.
This particular logic is already a bit complex so I decided to move out of it. I left the supported class due to what is written on next comment. Let me know what you think there.
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 agree that we shouldn't have instance methods mutating state, but this is something different. This is a static
factory method that belongs to ProjectCombinations
. I don't see any benefit in moving it out to a separate class.
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.
removed support class
import java.util.List; | ||
import java.util.Set; | ||
|
||
public class ProjectCombinations { |
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.
Why does this belong in a package called support
? This is part of the mutators.
If we could merge the two project-dependency-change-related mutators, we could move this to a nested subclass, and that'd look a lot better I think.
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.
Mutations package is really huge, and it's hard to find classes there. I created this package so these support classes, that are not mutators, are easy to find. They are still under mutations package.
I will check how easy it is to merge two classes into one.
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 merged mutator classes into one. Code wise it is probably easier to understand. I also moved ProjectCombinations as a subclass of now one mutator class.
But I kept the support class, that way since it's a bit complex logic and it's much nicer to test that if it is separate. And since it's easier to test that when separate unit, that is a good flag for me that it might deserve a separate class. WDYT?
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 like that ProjectCombinations
is now a nested class. I think that makes this support class even more of an orphan that should be reunited with its folks.
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.
removed support class
src/test/groovy/org/gradle/profiler/mutations/ApplyChangeToAndroidLayoutFileMutatorTest.groovy
Outdated
Show resolved
Hide resolved
@@ -47,6 +50,7 @@ class ApplyChangeToAndroidLayoutFileMutatorTest extends AbstractMutatorTest { | |||
def mutator = new ApplyChangeToAndroidLayoutFileMutator(sourceFile) | |||
|
|||
when: | |||
mutator.beforeScenario(scenarioContext) | |||
mutator.afterScenario(scenarioContext) |
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 think these tests don't make much sense now. We could make some random change between beforeScenario()
and afterScenario()
to make it meaningful.
But since this logic of "saving and restoring the original text" happens for every file change mutator, it's not really useful to test it so many times.
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.
Maybe a better option would be to have a method to create the mutator in every test that also calls mutator.beforeScenario()
and mutator.beforeBuild()
?
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 see your point., but there are actually tests with: reverts changes when nothing has been applied
:D. So they already tested before that there is no action.
I left them for now, since they are there, do you think I should delete them to not continue with that pattern in future?
Co-authored-by: Lóránt Pintér <lorant@gradle.com>
e331aa7
to
11dc2be
Compare
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.
LGTM, the remaining comments are about code style that we can discuss separately if we want.
@@ -1,12 +1,15 @@ | |||
package org.gradle.profiler.mutations.support |
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.
Shall we move this to a level higher? Or merge the test into ApplyProjectDependencyChangeMutatorTest
?
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.
Done
@@ -123,5 +139,78 @@ public ProjectCombinations(List<String> projectNames, Set<Set<String>> combinati | |||
public Set<String> getNextCombination() { | |||
return combinations.next(); | |||
} | |||
|
|||
public static ProjectCombinations createProjectCombinations(int numberOfRequiredCombinations, int appliedProjectsCount) { |
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.
No need for this to be public
(we typically use default visibility + @VisibleForTesing
for methods that need to be available from tests). The methods can also be part of ApplyProjectDependencyChangeMutator
itself, though that might be a question of taste.
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.
Removed public and add annotation
… also limit scope of ProjectCombinations creator method
We generate projects before scenario begins so we don't modify
settings.gradle
later. And then combinations of project dependencies are applied to the build.