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

BE: Fix KafkaConsumerGroupTests on Windows #261

Merged
merged 8 commits into from
Aug 1, 2024
Merged

Conversation

busches
Copy link
Contributor

@busches busches commented Apr 2, 2024

  • Breaking change? (if so, please describe the impact and migration path for existing application instances)

What changes did you make? (Give an overview)
shouldOkWhenConsumerGroupIsNotActive was returning a 400 instead of a 200 without the try with resources block, once I added it, the test consistently passes. Additionally removed some unused variables and used toList() added with JDK 17.

Is there anything you'd like reviewers to focus on?

How Has This Been Tested? (put an "x" (case-sensitive!) next to an item)

  • No need to
  • Manually (please, describe, if necessary)
  • Unit checks
  • Integration checks
  • Covered by existing automation

Checklist (put an "x" (case-sensitive!) next to all the items, otherwise the build will fail)

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation (e.g. ENVIRONMENT VARIABLES)
  • My changes generate no new warnings (e.g. Sonar is happy)
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged

Check out Contributing and Code of Conduct

A picture of a cute animal (not mandatory but encouraged)

shouldOkWhenConsumerGroupIsNotActive was returning a 400 instead of a 200 without the try with resources block, once I added it, the test consistently passes.
@busches busches requested a review from a team as a code owner April 2, 2024 03:14
@kapybro kapybro bot added status/triage Issues pending maintainers triage status/triage/manual Manual triage in progress status/triage/completed Automatic triage completed and removed status/triage Issues pending maintainers triage labels Apr 2, 2024
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Hello there busches! 👋

Thank you and congrats 🎉 for opening your first PR on this project! ✨ 💖

We will try to review it soon!

@busches busches changed the title Fix test on Windows Fix KafkaConsumerGroupTests on Windows Apr 2, 2024
@germanosin
Copy link
Member

Hi, @busches. Thanks for your contribution! Looks nice! @iliax don't you mind to review it?

@iliax
Copy link
Contributor

iliax commented Apr 2, 2024

@busches your changes looks fine, but I can't understand how they can fix tests on windows) Can you please add more context here and describe why this is working now?

@Haarolean Haarolean added scope/backend Related to backend changes type/chore Boring stuff, could be refactoring or tech debt status/feedback-requested and removed status/triage/manual Manual triage in progress labels Apr 2, 2024
Copy link

kapybro bot commented Apr 2, 2024

Further user feedback is requested. Please reply within 7 days or we might close the issue.

@kapybro kapybro bot assigned busches Apr 2, 2024
@busches
Copy link
Contributor Author

busches commented Apr 2, 2024

What the code change does is ensure that the Consumer Group is cleaned up, with the try with resources, before it executes the API request. The same is true for the other test, where we leave the Consumer Group up until after the API request. That test did not have any issues passing for me, but I changed it to mirror the other for self documenting purposes.

The issue may not be related to Windows at all, but that's what I'm running. It could be a case of my computer is too slow to remove the consumer group fully before the API request runs or potentially something with my JDK, I was using Oracle and will swap to Termium tonight. Note: there were three failing test suites I encountered, this one, one for the Protobuf stuff (unsure of the cause) and another that is 100% OS line endings that I'll fix next.

Copy link

kapybro bot commented Apr 2, 2024

Thanks for the additional feedback! We'll get back to your issue soon.

@kapybro kapybro bot unassigned busches Apr 2, 2024
@Haarolean
Copy link
Member

I don't personally mind a few try with resources here even if they don't fix anything as long as it's not PR spam.
@busches is there anything else you'd like to add up to this PR or should I merge it?

@busches
Copy link
Contributor Author

busches commented Apr 2, 2024

@Haarolean I would like to fix the other test failures I'm seeing on Windows, but no ETA on when I can get them in :)

@Haarolean Haarolean marked this pull request as draft April 2, 2024 14:49
Not replacing it, means `language/language.proto` is not found in the Map on Windows. Updating just the key, results in a duplicate file found with different path, as it still loads the import and from the directory pathing.
@busches busches marked this pull request as ready for review April 3, 2024 02:03
@busches
Copy link
Contributor Author

busches commented Apr 3, 2024

Everything is passing now on Windows. I didn't like that I had to change ProtobufFileSerde, the other option is to change the test files to not be nested in the language folder.

Copy link

sonarcloud bot commented Apr 3, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@Haarolean Haarolean self-assigned this May 1, 2024
@Haarolean Haarolean self-requested a review May 19, 2024 19:31
@Haarolean Haarolean changed the title Fix KafkaConsumerGroupTests on Windows BE: Fix KafkaConsumerGroupTests on Windows May 19, 2024
@Haarolean Haarolean merged commit 42b3ae8 into kafbat:main Aug 1, 2024
11 of 13 checks passed
@Haarolean
Copy link
Member

Thanks for a contribution and sorry for the delay

@busches busches deleted the patch-1 branch August 1, 2024 18:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope/backend Related to backend changes status/triage/completed Automatic triage completed type/chore Boring stuff, could be refactoring or tech debt
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants