Skip to content

refactor: replace JUnit assertion in ScramInitialiser - #673

Merged
robobario merged 2 commits into
kroxylicious:mainfrom
lntutor:fix/667-remove-junit-assertion
Aug 12, 2026
Merged

refactor: replace JUnit assertion in ScramInitialiser#673
robobario merged 2 commits into
kroxylicious:mainfrom
lntutor:fix/667-remove-junit-assertion

Conversation

@lntutor

@lntutor lntutor commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

Type of change

  • Refactoring

Description

Replaces the JUnit assertTrue call in ScramInitialiser production code with an explicit Java state check and a descriptive IllegalStateException.

Additional Context

This removes the production dependency on JUnit assertions while preserving Awaitility's retry behavior until every configured SCRAM user is visible.

Fixes #667

Checklist

  • Write tests
  • Make sure all tests pass
  • Update documentation (not needed)
  • Reference relevant issue(s) and close them after merging
  • Update CHANGELOG.md (not needed)

Verification

  • mvn -pl impl -am -Dtest=ScramInitialiserTest -Dsurefire.failIfNoSpecifiedTests=false process-sources test on Java 21

@lntutor
lntutor requested a review from a team as a code owner July 31, 2026 19:10
@lntutor

lntutor commented Aug 1, 2026

Copy link
Copy Markdown
Contributor Author

Current head 16e9e646ae66485b2a8b9a51adb2744d46b28638 is mergeable, the visible DCO check is green on that exact SHA, and there are no review threads or review-body comments open on the PR at the moment. If this assertion cleanup still matches the intended direction, it looks ready for maintainer review/merge when convenient.

@robobario robobario left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks @lntutor! The implementation looks good to me, but I'd rather merge with no unit test than the bytecode string comparison.

Comment thread impl/src/test/java/io/kroxylicious/testing/kafka/common/ScramInitialiserTest.java Outdated

@robobario robobario left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

Assisted-by: OpenAI Codex GPT-5.6 <noreply@openai.com>
Signed-off-by: Loi Nguyen <vanloi999@gmail.com>
Assisted-by: OpenAI Codex GPT-5 <noreply@openai.com>
Signed-off-by: Loi Nguyen <vanloi999@gmail.com>
@robobario
robobario force-pushed the fix/667-remove-junit-assertion branch from 7718d16 to b62d483 Compare August 12, 2026 00:25
@sonarqubecloud

Copy link
Copy Markdown

@robobario
robobario merged commit fe81ae0 into kroxylicious:main Aug 12, 2026
10 checks passed
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.

Replace JUnit Assertions.assertTrue in ScramInitialiser with plain Java check

2 participants