Test: Update pr-java/functional-ci work flow
#3904
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
marki1an
added
enhancement
marki1an
changed the title
pr-java-ci work flowpr-java/functional-ci work flow
This reverts commit 3d41b53.
osulzhenko
previously approved these changes
Jun 3, 2025
|
This is great 🥳 Do the functional test not produce junit reports, so we could leverage existing actions like https://github.com/marketplace/actions/junit-report-action or https://github.com/marketplace/actions/test-reporter ? |
This reverts commit 5a9ae73.
|
Hey @muuki88 ,
Could you share what specific advantages you're seeing with the current setup that might outweigh these concerns? I'm open to discussing best approach for community. |
|
@marki1an, could you please take a look at the functional test failures? [ERROR] Tests run: 4442, Failures: 1, Errors: 0, Skipped: 24 Show failed tests details |
|
@marki1an, could you please take a look at the functional test failures? [ERROR] Tests run: 4442, Failures: 1, Errors: 0, Skipped: 24 Show failed tests details |
|
Hi @osulzhenko Thanks for the feedback 🙏 You are right, I was a little bit short on the why
From my experience it's often easier to build upon standards (junit test reports + a reporter) instead of glueing different tools together (sed, awk, action to comment on PRs, ... ) . The implementation currently works and I haven't nothing against bash scripts - however if the output of any of the test frameworks used just slightly changes, this integration will likely break. Trying to re-use existing standards reduces that risk. I wouldn't feel confident maintaining this implementation. I would at least move the inline bash script into a separate bash script to make it easy to test locally. Regarding your concerns
If you break more than 50 tests, I'm not sure how much more critical issues can be hidden 😅 The idea is that there are no failing tests before a PR is merged, right? 😁
The currently solution is no different, right? There are just a lot of comments. And I do like the annotations, because it directly guides me to the issue.
What I do however find irritating is
I agree. What do you mean by maintaining two systems, though? Currently there are three bash scripts, which I would argue are three different systems, which are not configured, but actually implemented, making maintenance even higher. If you mean the two links I posted - I would assume that one of the two is enough. Just don't know which one is better. https://github.com/marketplace/actions/test-reporter looks a little more comprehensive. The summaries also look pretty decent
|
|
We have three different types of test result reporting. To keep it DRY and clean, I believe we should finalize on one variant. Examples of each: Bash: Mikepenz: Dorny: Personally, I prefer bash because it’s simple, clear, and self-explanatory. Mikepenz tends to create some noise, especially with PR comments. Dorny doesn’t have full support for java yet, but it provides a report with timing for each spec, which is quite helpful. Other alternative reporting tools I found aren’t very useful. So, for me, it’s pretty simple: either bash or dorny, combined with a PR message linking to report showing failed tests. @marki1an @muuki88 @Net-burst What do you think? |
|
@marki1an, could you please take a look at the functional test failures? [ERROR] Tests run: 4792, Failures: 1, Errors: 0, Skipped: 24 Show failed tests details |
Yeah, I agree with you — Bash is much simpler and easier to extend with custom logic if needed. Additionally, it will post a message in the pull request, allowing you to see if anything went wrong. |
I wouldn't agree on this. In the end I just wanna see what's broken and use a standard format to display the results, like junit.. Fiddling with bash is the last thing I want to do 😅 |
|
I will also pop up here with an opinion :D Also I'll put my own signature under that:
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
🔧 Type of changes
✨ What's the context?
What's the context for the changes?
🧠 Rationale behind the change
Why did you choose to make these changes? Were there any trade-offs you had to consider?
🔎 New Bid Adapter Checklist
🧪 Test plan
How do you know the changes are safe to ship to production?
🏎 Quality check