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

Added JUnit parser #70

Merged
merged 1 commit into from Jun 15, 2019

Conversation

@chemfy
Copy link
Contributor

commented Jun 4, 2019

No description provided.

@tomasbjerre

This comment has been minimized.

Copy link
Owner

commented Jun 4, 2019

I'm not sure about this. Is JUnit really static code analysis?

What is the use case? Are there not other tools supporting same use case?

@chemfy

This comment has been minimized.

Copy link
Contributor Author

commented Jun 4, 2019

Hello. No, JUnit is not static analysis.
I started using the top level jenkins plugin to get build results to get posted on pull requests. I was surprised that the plugin didn't support posting test results. I considered my options for getting this support and thought the easiest would be to get it added here.

I see the use case as the same as for the static analysis. To make sure the pull request creator notices those. I haven't found any other tool for this. Every other tool requires the user to goto Jenkins to see what is wrong.

I don't see why this should only be about static analysis, but vision/usage of it may not be the same as yours.

@tomasbjerre

This comment has been minimized.

Copy link
Owner

commented Jun 4, 2019

"...requires the user to goto Jenkins to see what is wrong" that is a good point!

And you are aware that only changed files will currently be commented? So the failed test case has to have been touched.

Or this also requires https://github.com/tomasbjerre/violation-comments-lib to be further developed to support some option like commentWithEveryViolation that would not only comment changed files, but comment with every violation.

@chemfy

This comment has been minimized.

Copy link
Contributor Author

commented Jun 4, 2019

I'm currently using the publisher to bitbucket with "commentOnlyChangedContent: false". Which I think would ensure that all failures would be commented.
But no, I did not consider that. Should it be some type of ability to have, parser specific custom settings that the use could configure?

@tomasbjerre

This comment has been minimized.

Copy link
Owner

commented Jun 4, 2019

I don't think it has to be parser specific. It can be on same level as commentOnlyChangedContent. Assuming most users are using pipelines, they can just use the plugin twice if they want different configs for different parsers.

Regarding the behavior of commentOnlyChangedContent. It changes if all violations found on a changed file should be commented or only the violations found in the changed content of the file.

@tomasbjerre

This comment has been minimized.

Copy link
Owner

commented Jun 5, 2019

Actually, perhaps commentOnlyChangedFiles is a better name than commentWithEveryViolation to use in violation-comments-lib. And have it true by default.

So in this case it would be set to false and have every violation commented.

@chemfy

This comment has been minimized.

Copy link
Contributor Author

commented Jun 5, 2019

Oh ok, I misunderstood how commentOnlyChangedContent worked, but now I know.

I can look at adding that to violation-comments-lib over the weekend.

@tomasbjerre

This comment has been minimized.

Copy link
Owner

commented Jun 7, 2019

Sounds good.

The method void createCommentWithAllSingleFileComments(String string); here:
https://github.com/tomasbjerre/violation-comments-lib/blob/master/src/main/java/se/bjurr/violations/comments/lib/CommentsProvider.java

Can probably be renamed to void createPullRequestComment(String string);.

As that is what it does, it creates a comment on the PR, not on a file. And then use that method for the commenting.

Also, try not to refactor the API (even if it is far from perfect) as that will result in a lot of docs to be updated and most likely future support issues. This should be possible to implement and still being totally backwards compatible.

@tomasbjerre tomasbjerre force-pushed the tomasbjerre:master branch from 3e2961e to d5e57cf Jun 11, 2019
@tomasbjerre tomasbjerre merged commit 1310007 into tomasbjerre:master Jun 15, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
tomasbjerre added a commit that referenced this pull request Jun 15, 2019
@tomasbjerre

This comment has been minimized.

Copy link
Owner

commented Jun 15, 2019

Released in 1.94

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.