gradle-baseline icon indicating copy to clipboard operation
gradle-baseline copied to clipboard

Revert "Don't apply JUnit reports plugin by default (#2355)"

Open CRogers opened this issue 3 years ago • 2 comments

This reverts commit a79fec7f452c61cfaa1a51275d5dce1af8f729f8 / #2355.

Before this PR

In #2355 this was reverted. However, I think this has made the dev experience quite a lot worse. We've had complaints internally this feature is broken and I personally have now found myself trawling through build output rather than just being able to look at checkstyle/compilation error/gradle task failures at the top of the build. Sure it may be a bit odd to see them called tests - but I think this downside is weighed by the big upside of saving devs time and listing out the errors one by one in an easy to consume format. Note, even in circle 2 times these were called failing tests.

After this PR

==COMMIT_MSG== Restore the JUnit reports plugin by default so checkstyle, compile errors and gradle task failures appear in an easy to read format on Circle. ==COMMIT_MSG==

Possible downsides?

If there are downsides to this plugin I argue we should try to improve it rather than removing the functionality (and in my view this is a critical piece of dev time saving).

CRogers avatar Sep 16 '22 15:09 CRogers

Generate changelog in changelog/@unreleased

Type

  • [ ] Feature
  • [ ] Improvement
  • [x] Fix
  • [ ] Break
  • [ ] Deprecation
  • [ ] Manual task
  • [ ] Migration

Description Restore the JUnit reports plugin by default so checkstyle, compile errors and gradle task failures appear in an easy to read format on Circle.

Check the box to generate changelog(s)

  • [x] Generate changelog entry

changelog-app[bot] avatar Sep 16 '22 15:09 changelog-app[bot]

While not my preference, I can see the benefit of creating test reports for Gradle tasks.

But there were some legitimate issues fixed in https://github.com/palantir/gradle-baseline/pull/2355 that I would strongly prefer we do not revert. Namely:

  • The plugin creating a never ending amount of build.xml garbage.
  • Storing test reports in a non-standard place.
    • It is confusing that they are in a folder called junit-reports.
    • It is strange that we move them to the root project instead of just keeping them in the subproject build directories.

More details about the motivation for these parts of the change can be found in the description of https://github.com/palantir/gradle-baseline/pull/2355.

pkoenig10 avatar Sep 16 '22 18:09 pkoenig10

@CRogers Can we please fix this without regressing on the improvements from https://github.com/palantir/gradle-baseline/pull/2355?

pkoenig10 avatar Nov 08 '22 16:11 pkoenig10

From a user's perspective, the regression from not having this feature enabled has been quite extreme internally - I've certainly found myself missing it, as well as others. I'm going to revert back to good from their perspective. From then we can look at improving the internals of the plugin, however given our other priorities on our team we're unlikely to be able to invest our own time into this any time soon.

I also think one of the issues (returning identical task failures from many circle nodes) highlights another problem we need to fix - the fact these tasks run multiple times on every node rather than just one node. There's no need to run spotlessApply etc 8 times if you have 8 nodes. Even then most projects don't have check task parallelism more than 1, so don't run into this issue.

The build.xml proliferation is not great, but I honestly don't think most devs notice it.

CRogers avatar Nov 08 '22 16:11 CRogers

Closing in favor of https://github.com/palantir/gradle-baseline/pull/2724

pkoenig10 avatar Feb 23 '24 18:02 pkoenig10