Skip to content

Add unit tests to support #838#843

Merged
k-wall merged 3 commits intokroxylicious:mainfrom
k-wall:issue-838-unittests
Jan 10, 2024
Merged

Add unit tests to support #838#843
k-wall merged 3 commits intokroxylicious:mainfrom
k-wall:issue-838-unittests

Conversation

@k-wall
Copy link
Copy Markdown
Member

@k-wall k-wall commented Dec 22, 2023 •
edited by SamBarker
Loading

Type of change

Select the type of your PR

  • Enhancement

Description

Unit tests to accompany fix for #838.

Verified that the new test fails in the expected way if you backout the fix for #838.

Screenshot 2023-12-22 at 12 57 04

Additional Context

Why are you making this pull request?

Checklist

Please go through this checklist and make sure all applicable tasks have been done

  • Write tests
  • Make sure all tests pass
  • Review performance test results. Ensure that any degradations to performance numbers are understood and justified.
  • Make sure all Sonar-Lint warnings are addressed or are justifiably ignored.
  • Update documentation
  • Reference relevant issue(s) and close them after merging
  • For user facing changes, update CHANGELOG.md (remember to include changes affecting the API of the test artefacts too).

@k-wall k-wall added the test Relates to testing label Dec 22, 2023
k-wall
k-wall commented Dec 22, 2023
View reviewed changes
Comment thread ...-encryption/src/test/java/io/kroxylicious/filter/encryption/inband/InBandKeyManagerTest.java
var kekId1 = kms.generateKey();
var kekId2 = kms.generateKey();

var spyKms = Mockito.spy(kms);
Copy link
Copy Markdown
Member Author

@k-wall k-wall Dec 22, 2023

Choose a reason for hiding this comment

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

😱

Copy link
Copy Markdown
Member Author

@k-wall k-wall Dec 22, 2023

Choose a reason for hiding this comment

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

Hideous .. I'm hoping once we've got #774 etc we'd be able to rewrite this tests using mocks, and avoid the scary spying nonsense.

Copy link
Copy Markdown
Member

@SamBarker SamBarker Jan 8, 2024

Choose a reason for hiding this comment

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

Ohh that might be the forcing function I need to to actually merge #774

@k-wall k-wall force-pushed the issue-838-unittests branch 3 times, most recently from bed571e to 5200109 Compare December 22, 2023 13:33
@k-wall k-wall linked an issue Dec 22, 2023 that may be closed by this pull request
Wrong message ordering through proxy with encryption enabled #838
Closed
@k-wall k-wall force-pushed the issue-838-unittests branch 2 times, most recently from efbd7d1 to a7d9087 Compare December 22, 2023 16:40
SamBarker
SamBarker approved these changes Jan 8, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@SamBarker SamBarker left a comment

Choose a reason for hiding this comment

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

LGTM,

Approved assuming the build is fixed by cosmetic changes.

Comment thread ...-encryption/src/test/java/io/kroxylicious/filter/encryption/inband/InBandKeyManagerTest.java
Comment thread ...-encryption/src/test/java/io/kroxylicious/filter/encryption/inband/InBandKeyManagerTest.java
var kekId1 = kms.generateKey();
var kekId2 = kms.generateKey();

var spyKms = Mockito.spy(kms);
Copy link
Copy Markdown
Member

@SamBarker SamBarker Jan 8, 2024

Choose a reason for hiding this comment

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

Ohh that might be the forcing function I need to to actually merge #774

@k-wall k-wall force-pushed the issue-838-unittests branch from a7d9087 to ebceae7 Compare January 10, 2024 10:12
@k-wall k-wall force-pushed the issue-838-unittests branch from ebceae7 to 4341e54 Compare January 10, 2024 10:18
@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud Bot commented Jan 10, 2024

Quality Gate Failed Quality Gate failed

Failed conditions

50.0% Coverage on New Code (required ≥ 76%)

See analysis details on SonarCloud

@k-wall k-wall merged commit 9e50e0c into kroxylicious:main Jan 10, 2024
@k-wall
Copy link
Copy Markdown
Member Author

k-wall commented Jan 10, 2024

Merged despite the coverage gate failure. The coverage gate failed because of the need to increase the visibility of the InMemory* record classes. Hopefully, we'll be able to revert this part of the change in future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@SamBarker SamBarker SamBarker approved these changes

Assignees

No one assigned

Labels

test Relates to testing

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Wrong message ordering through proxy with encryption enabled

2 participants

@k-wall @SamBarker