Skip to content

Conversation

vanessamj99
Copy link

A new file was created to report ktlint issues to the problems API to act as another option of error reporting. A testing file was also added to test the functionality of the new integration using mockito.

@vanessamj99
Copy link
Author

Hi, could I get some eyes on this pr? Thanks in advance! @Tapchicoma @JLLeitschuh @wakingrufus

@wakingrufus
Copy link
Collaborator

Thanks for the PR! Please give us some time to look this over, as it is a fairly impactful change. I also would like to have some TestKit integration tests for this, and perhaps with those, we would not have a need for the current test that use mocks (depending on what we are able to assert in a TestKit test)

Copy link
Owner

@JLLeitschuh JLLeitschuh left a comment

Choose a reason for hiding this comment

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

I'm finding myself confused, it's not clear to me how the ProblemsApiReporter is actually used in the codebase in this PR. The ProblemsApiReporter is never instantiated via dependency injection, and doesn't seem to be properly wired into Gradle's problems API in any meaningful way.

Am I missing something? I'm finding myself confused here.

@vanessamj99
Copy link
Author

Sorry about that! When I was creating the no op constructor it was supposed to be covering when the problems api couldn't be used because of the gradle version but I fixed your points and injected the problems and got rid of the constructors.

Copy link
Owner

@JLLeitschuh JLLeitschuh left a comment

Choose a reason for hiding this comment

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

I agree with @wakingrufus, I'd like to see at least one integration test for this change before I'm willing to merge this.

The current test as they stand do not capture whether or not this logic functions correctly within the context of the use of the plugin. The current tests only capture whether or not the new ProblemsApiReporter calls the correct Gradle APIs.

Copy link

@oleg-nenashev oleg-nenashev left a comment

Choose a reason for hiding this comment

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

@JLLeitschuh
Copy link
Owner

Overall, I'm finding myself quite confused about these changes and the back-and-forth we had to engage with on this PR. The vibe I was getting for many of the changes made in this PR were that the code generated was coming from an AI. The original set of changes didn't seem to understand the nuance of what was actually being implemented, nor following the repositories norms around testing things. The CHANGELOG changes don't actually properly flag what was changed. The original set of changes didn't even function properly, and were testing logic in such a way that was self-validating rather than actually validating the changes were implementing the designated feature. I'm happy to educate someone on how to implement features in this repository, but this whole PR review process felt weird and outside of the norm I'm used to experiencing, so I'm trying to understand what was actually going on here.

Overall feedback: in the future, if using AI, please utilize critical thinking around the changes proposed to ensure that the changes are actually implementing the feature desired.

@vanessamj99
Copy link
Author

The reason I used mockito is because that is the testing framework I usually used but I see I should have used the testing already done in this repo. I was implementing this in a different repo but is different here. Will fix the changelog and README.

@JLLeitschuh
Copy link
Owner

This PR has conflicts with main, can you please rebase the changes?

@oleg-nenashev
Copy link

@JLLeitschuh it was merged

Copy link
Owner

@JLLeitschuh JLLeitschuh left a comment

Choose a reason for hiding this comment

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

One simple change requested. I'm also going to run the unit tests. Assuming the tests pass, and this change is made, I'm good to merge this.

@vanessamj99
Copy link
Author

Sorry about the delay! I was and am traveling for a conference. I'll look back into this in the next couple of days.

@JLLeitschuh
Copy link
Owner

No worries. Could you rebase when you get a moment?

@vanessamj99
Copy link
Author

I rebased and I was looking at back the integration tests and deleted the file since it doesn't directly test the integration of the problems api. Not sure if there is an easy way to test the integration.

@JLLeitschuh
Copy link
Owner

Per this conversation in the Gradle Community slack channel: https://gradle-community.slack.com/archives/CA745PZHN/p1758831121015639

Would you be willing to implement an integration test that verifies the output of the problems API ends up in the report file that is created when using this API?

Related:

@JLLeitschuh
Copy link
Owner

Also, you can safely ignore the "Build and check / build (ubuntu-latest, 25)" and "Build and check / build (windows-latest, 25)" CI job failures. Those are known issues. See: #963

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.

5 participants