Feature/separate plugin config and execution #1542

l-1squared · 2024-02-19T06:14:53Z

TODO:

~~test that the jgiven report doesn't produce results if neither test, nor report is requested~~
~~check how the report looks with that double task instruction~~
~~better message for WIP commit~~
~~changelog~~
~~credit in changelog~~

l-1squared · 2024-02-19T06:17:15Z

jgiven-gradle-plugin/src/test/java/com/tngtech/jgiven/gradle/JGivenPluginTest.java

reconsider what the formatter did here :(

l-1squared · 2024-02-19T06:22:36Z

jgiven-gradle-plugin/src/test/java/com/tngtech/jgiven/gradle/JGivenPluginTest.java

-            .a_build().with()
-            .the_task("jgivenTestReport").is_successful();
+                .a_build().with()
+                .the_task("-x").the_task("test").and()


@jjohannes I'd like to, for one final time, have your opinon on this one.
To me it seems, now that we properly use Providers, Gradle is able to track the necessary inputs of the JGiven task.
Therefore, previously a JGiven report task would not run, if a test task was not explicitely requested. Now, I assume that because we explicitely request the report task to run, Gradle knows that it needs to run the test task and does so accordingly.

While this is changed behavior, I am of the opinon, that we are now more in line with how Gradle is expected to behave and therefore this would generally be an appreciated change.

How do you see this?

Yes, your observation is correct. The providers that point at task outputs carry the task dependency information and that is why the test task is automatically executed before.

If you can accept the breaking change, I would agree that this is the better behavior. It follows Gradle best practices. (If not, there would be ways to "trick" the setup into loosing the dependency but still using providers in general.)

Note: Traditionally the jacoco report task built into Gradle core also does not follow this best practice. However I believe this is due to not wanting to break the behavior that was introduced early (pre Gradle 1.0). If the JaCoCo tasks/plugin would be created today, I think it would also behave different.

jjohannes · 2024-02-21T12:12:06Z

jgiven-gradle-plugin/src/main/java/com/tngtech/jgiven/gradle/JGivenReportTask.java

+    protected ObjectFactory getObjects() {
        throw new UnsupportedOperationException();
    }


Suggested change

protected ObjectFactory getObjects() {

throw new UnsupportedOperationException();

}

protected abstract ObjectFactory getObjects();

Just a suggestion to make it "more compact".

jjohannes · 2024-02-21T12:13:13Z

jgiven-gradle-plugin/src/main/java/com/tngtech/jgiven/gradle/JGivenPlugin.java

+        reportTask.getResults().set(getResultsDirectory);
+
+        String relativeFilePath = "jgiven" + "/" + test.getName() + "/";
+        Provider<Directory> reportOutputLocation= reportingExtension.getBaseDirectory().dir(relativeFilePath);


Suggested change

Provider<Directory> reportOutputLocation= reportingExtension.getBaseDirectory().dir(relativeFilePath);

Provider<Directory> reportOutputLocation = reportingExtension.getBaseDirectory().dir(relativeFilePath);

jjohannes · 2024-02-21T15:54:26Z

jgiven-gradle-plugin/src/main/java/com/tngtech/jgiven/gradle/JGivenPlugin.java

+        Provider<Directory> getResultsDirectory = test.getExtensions()
+                .getByType(JGivenTaskExtension.class)
+                .getResultsDir();
+
+        reportTask.getResults().set(getResultsDirectory);


Suggested change

Provider<Directory> getResultsDirectory = test.getExtensions()

.getByType(JGivenTaskExtension.class)

.getResultsDir();

reportTask.getResults().set(getResultsDirectory);

Provider<Directory> resultsDirectory = test.getExtensions()

.getByType(JGivenTaskExtension.class)

.getResultsDir();

reportTask.getResults().convention(resultsDirectory);

Using convention makes this the "fall-back" value. Then you can be sure that if a user re-configures this, the user value will override the default. Changing the value here also allows users to "break" the task dependency to the test task if they are not happy with that change.

Funnily enought, I had this as convention for a while, but convinced myself that "set" was correct ^^

Signed-off-by: l-1squared <30831153+l-1squared@users.noreply.github.com>

these have been detailed https://github.com/jjohannes/JGiven/commit/cc3f67a7b4b39f9f78599c2cea18c42c95870c73 and authority to use these has been explicitly granted #1527 (comment) Signed-off-by: l-1squared <30831153+l-1squared@users.noreply.github.com>

turns out that this check is not what prevented the task from running and it is neither the "SkipWhenEmpty" instruction on the input. Rather, for some reason unknown to me, the test task did not run, but now it does. Signed-off-by: l-1squared <30831153+l-1squared@users.noreply.github.com>

…o using conventions Signed-off-by: l-1squared <30831153+l-1squared@users.noreply.github.com>

This commit is a reimplementation from dangling commit 37c736e of this repository, which I couldn't get to cherry pick. The original commit was done by jjohannes and the message reads "Move report location computation out of 'confiugureEach'" Signed-off-by: l-1squared <30831153+l-1squared@users.noreply.github.com>

I guess we don't need that anymore, now that we properly mapped in / and outputs via providers Signed-off-by: l-1squared <30831153+l-1squared@users.noreply.github.com>

…Given result Reason: Apparently, now that we use providers gradle can map the in and outputs of the jgiven task and hence requires a test task to run when a JGiven task is requested. Signed-off-by: l-1squared <30831153+l-1squared@users.noreply.github.com>

…d test tasks. A lot of those are technically testing the gradle API, but since handling that API is fiddly, and it is easy to do it wrong, I opt to rather have an additional assertion on how the Jgiven task operates. Especially for the case that we lay out in our example projects Signed-off-by: l-1squared <30831153+l-1squared@users.noreply.github.com>

Signed-off-by: l-1squared <30831153+l-1squared@users.noreply.github.com>

to highlight that these are github usernames. Also these changes have been made through the entire document to be consistent. Signed-off-by: l-1squared <30831153+l-1squared@users.noreply.github.com>

Signed-off-by: l-1squared <30831153+l-1squared@users.noreply.github.com>

and minor code beatification from review comments Signed-off-by: l-1squared <30831153+l-1squared@users.noreply.github.com>

fudler

Two small comments added.

fudler · 2024-02-26T07:27:10Z

jgiven-gradle-plugin/src/test/groovy/com/tngtech.jgiven.gradle/JGivenPluginShould.groovy

+        when:
+        def result = GradleRunner.create()
+                .withProjectDir(testProjectDir)
+                .withArguments("--configuration-cache", "test", "jgivenTestReport", "-S", "--info"/*, "-Dorg.gradle.debug=true"*/ )


Will this comment still have some purpose?

You could remove that comment but add this line:

.withDebug(ManagementFactory.getRuntimeMXBean().getInputArguments().toString().contains("-agentlib:jdwp"))

Then, when you start a test in debug in IntelliJ, it will run the "Gradle under test" in the same process that you are debugging. And you can debug inside plugin.

Damn, I totally missed that. but @jjohannes thanks for the suggestion

fudler · 2024-02-26T07:27:29Z

jgiven-gradle-plugin/src/main/java/com/tngtech/jgiven/gradle/JGivenPlugin.java

-import org.gradle.api.Task;
-import org.gradle.api.internal.ConventionMapping;
-import org.gradle.api.internal.IConventionAware;
+import org.gradle.api.*;


Could this import be avoidable?

l-1squared self-assigned this Feb 19, 2024

l-1squared commented Feb 19, 2024

View reviewed changes

l-1squared force-pushed the feature/separate-plugin-config-and-execution branch from d4b8874 to 8fbb431 Compare February 20, 2024 05:46

jjohannes approved these changes Feb 21, 2024

View reviewed changes

jjohannes reviewed Feb 21, 2024

View reviewed changes

l-1squared added 12 commits February 23, 2024 06:24

Issue-1527: create a test that fails due to the config cache

371035c

Signed-off-by: l-1squared <30831153+l-1squared@users.noreply.github.com>

Issue-1527: use a property on the task extension

6ffb5ef

Signed-off-by: l-1squared <30831153+l-1squared@users.noreply.github.com>

Issue-1527: Move back from setting the results in the configuration t…

53630c1

…o using conventions Signed-off-by: l-1squared <30831153+l-1squared@users.noreply.github.com>

Issue-1527: remove manual must run after declaration

24f06a7

I guess we don't need that anymore, now that we properly mapped in / and outputs via providers Signed-off-by: l-1squared <30831153+l-1squared@users.noreply.github.com>

Issue-1527: write changelog

92f1449

Signed-off-by: l-1squared <30831153+l-1squared@users.noreply.github.com>

Issue-1527: put an "@" in front of all usernames in the changelog

02df77b

to highlight that these are github usernames. Also these changes have been made through the entire document to be consistent. Signed-off-by: l-1squared <30831153+l-1squared@users.noreply.github.com>

Issue-1527: JGiven Test now handles arguments with spaces nicely

ee64ff2

Signed-off-by: l-1squared <30831153+l-1squared@users.noreply.github.com>

l-1squared force-pushed the feature/separate-plugin-config-and-execution branch from 802894f to ee64ff2 Compare February 23, 2024 05:26

Issue-1527: Make results mapping a convention

9be2e78

and minor code beatification from review comments Signed-off-by: l-1squared <30831153+l-1squared@users.noreply.github.com>

l-1squared marked this pull request as ready for review February 23, 2024 05:32

l-1squared requested a review from fudler February 23, 2024 07:38

fudler approved these changes Feb 26, 2024

View reviewed changes

l-1squared merged commit e699d4f into master Feb 29, 2024
20 checks passed

l-1squared deleted the feature/separate-plugin-config-and-execution branch February 29, 2024 05:52

l-1squared linked an issue Feb 29, 2024 that may be closed by this pull request

Gradle plugin not compatible with Gradle configuration cache #1527

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/separate plugin config and execution #1542

Feature/separate plugin config and execution #1542

l-1squared commented Feb 19, 2024 •

edited

Loading

l-1squared Feb 19, 2024

l-1squared Feb 19, 2024

jjohannes Feb 19, 2024

jjohannes Feb 21, 2024

jjohannes Feb 21, 2024

jjohannes Feb 21, 2024

jjohannes Feb 21, 2024 •

edited

Loading

l-1squared Feb 23, 2024

fudler left a comment

fudler Feb 26, 2024

jjohannes Feb 26, 2024

l-1squared Mar 1, 2024

fudler Feb 26, 2024

	Provider<Directory> reportOutputLocation= reportingExtension.getBaseDirectory().dir(relativeFilePath);
	Provider<Directory> reportOutputLocation = reportingExtension.getBaseDirectory().dir(relativeFilePath);

Feature/separate plugin config and execution #1542

Feature/separate plugin config and execution #1542

Conversation

l-1squared commented Feb 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjohannes Feb 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fudler left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

l-1squared commented Feb 19, 2024 •

edited

Loading

jjohannes Feb 21, 2024 •

edited

Loading