Skip to content
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 com.github.vlsi.gradle-extensions to make test output human-readable #349

Merged
merged 1 commit into from
Jun 10, 2022

Conversation

vlsi
Copy link
Contributor

@vlsi vlsi commented May 28, 2022

Overview

I might be biased, however, I believe https://github.com/vlsi/vlsi-release-plugins/blob/master/plugins/gradle-extensions-plugin/README.md#test-output-formatting makes CI output much easier to understand.


I hereby agree to the terms of the jqwik Contributor Agreement.

@jlink
Copy link
Collaborator

jlink commented May 28, 2022

I assume that also works for output in GitHub actions?

@vlsi
Copy link
Contributor Author

vlsi commented May 28, 2022

Yeah, I mainly implemented the plugin for improving GitHub Actions output.
Windows terminal does not support coloring, however, macOS and Linux terminals will be colored.

@vlsi
Copy link
Contributor Author

vlsi commented May 28, 2022

Here's a sample failure: https://github.com/pgjdbc/pgjdbc/actions/runs/2299608243 (try opening test jobs)

@vlsi vlsi force-pushed the gradle-extensions branch from 5857630 to 2006b38 Compare May 28, 2022 13:01
@vlsi
Copy link
Contributor Author

vlsi commented May 28, 2022

I just pushed a commit that would trigger test failure, so we could see what it would look like in the Actions UI.

@vlsi
Copy link
Contributor Author

vlsi commented May 28, 2022

I think the output is much better, however, it might make sense to exclude successful tests faster than 0.5s from the output.

WDYT?

@jlink
Copy link
Collaborator

jlink commented May 28, 2022

WDYT?

I personally prefer to only see failures (and maybe warnings) in the output. Everything else is noise to me.

@vlsi vlsi force-pushed the gradle-extensions branch from db03a8b to c2a2457 Compare May 28, 2022 15:43
@vlsi
Copy link
Contributor Author

vlsi commented May 28, 2022

I personally prefer to only see failures (and maybe warnings) in the output

I agree, however, it is suspicious if a single test takes noticeable time (e.g. 30 seconds).

I've adjusted the threshold of slow tests to 15sec, and the output is cleaner now: https://github.com/jlink/jqwik/runs/6637715794?check_suite_focus=true#step:5:1295

I believe printing tests exceeding 15 sec might help capture cases like accidental algorithmic complexity like in CombinedIterator. Of course, 15 sec is adjustable, and you might have better idea what threshold would work for jqwik better.

@vlsi vlsi force-pushed the gradle-extensions branch 2 times, most recently from f9d4710 to fada0b3 Compare June 3, 2022 08:06
Comment on lines 1 to 8
plugins {
// See https://github.com/vlsi/vlsi-release-plugins/blob/master/plugins/gradle-extensions-plugin/README.md
id 'com.github.vlsi.gradle-extensions'
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could the syntax from below be used here:

project.plugins.apply('com.github.vlsi.gradle-extensions')

But I might be confusing different Gradle concepts...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gradle's recommendation is to mention all the plugins in the first plugins block, and they recommend id(..) syntax (see https://docs.gradle.org/current/userguide/plugins.html#sec:plugins_block ).

At the same time, applying plugins via apply call is discouraged (see https://docs.gradle.org/current/userguide/plugins.html#sec:old_plugin_application )

I do not follow why you suggest using deprecated techniques, especially when keeping #281 in mind.

In any case, I do not care regarding the exact syntax you prefer, and it is hard for me to understand the reasons you prefer project.plugins.apply, so feel free to adjust the code the way you like.

If you absolutely want me to do the change, of course, I can do it, however, it looks like a waste of time from my point of view.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if the trade-offs are different in shared configurations?

Anyway, at least it should be consistent. Let's ask the creator of the shared configs:
@tmohme Why did you use project.plugins.apply instead of plugins { id '...' } in #282?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe, that was by mistake. If you check https://github.com/jlink/jqwik/pull/282/files, you'll see that the PR added a lot plugins {..} calls, and project.plugins.apply was just in two places. They beg for moving into plugins{...}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case, I'd vote for moving everything to plugins{..} in this PR.

Copy link
Contributor

@tmohme tmohme Jun 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be honest, I don't remember for sure why I used the project.plugins.apply syntax in the convention plugins (as opposed to the plugins-block syntax everywhere else).
Most likely I've seen it somewhere in the gradle docs, maybe there was a limitation some time ago, maybe it was a simple mistake . . .
Anyway, I don't see any advantages in keeping the project.plugins.apply syntax right now and also would recommend the plugins {...} syntax.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tmohme Thanks for clarifying!

@vlsi vlsi force-pushed the gradle-extensions branch 2 times, most recently from 7507033 to 3e8fac8 Compare June 4, 2022 15:39
@jlink
Copy link
Collaborator

jlink commented Jun 10, 2022

@vlsi You'll have to rebase since I switched to plugins DSL in shared configurations

@vlsi vlsi force-pushed the gradle-extensions branch from 3e8fac8 to b561a13 Compare June 10, 2022 10:46
@jlink jlink merged commit 584b186 into jqwik-team:main Jun 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants