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

[improve][broker] Add test to verify authRole cannot change #19430

Merged
merged 2 commits into from
Feb 5, 2023

Conversation

michaeljmarshall
Copy link
Member

Motivation

While working on #19409, I noticed we do not have any tests in the ServerCnxTests class verifying the authRole cannot change. This seems like an oversight, and this PR adds tests to assert the existing behavior works as intended.

Modifications

  • Add new test AuthenticationProvider and AuthenticationState implementations to simplify testing
  • Add two tests to verify the behavior that a connection is closed when the authRole changes.

Verifying this change

This change is to add tests.

Documentation

  • doc-not-needed

Matching PR in forked repository

PR in forked repository: Skipping testing in my fork since these are new tests and I got them to pass locally.

@michaeljmarshall michaeljmarshall added this to the 3.0.0 milestone Feb 5, 2023
@michaeljmarshall michaeljmarshall self-assigned this Feb 5, 2023
Copy link
Member

@lhotari lhotari left a comment

Choose a reason for hiding this comment

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

Good work @michaeljmarshall

@codecov-commenter
Copy link

Codecov Report

Merging #19430 (3c8ef5e) into master (7449baa) will increase coverage by 0.02%.
The diff coverage is 81.25%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #19430      +/-   ##
============================================
+ Coverage     62.63%   62.66%   +0.02%     
- Complexity    25752    25768      +16     
============================================
  Files          1832     1832              
  Lines        134053   134067      +14     
  Branches      14752    14753       +1     
============================================
+ Hits          83967    84014      +47     
+ Misses        42346    42340       -6     
+ Partials       7740     7713      -27     
Flag Coverage Δ
inttests 24.85% <43.75%> (+0.01%) ⬆️
systests 25.42% <43.75%> (-0.01%) ⬇️
unittests 59.90% <81.25%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...rg/apache/pulsar/client/impl/PulsarClientImpl.java 73.65% <ø> (-1.21%) ⬇️
.../client/admin/internal/PulsarAdminBuilderImpl.java 87.67% <78.57%> (-2.16%) ⬇️
...g/apache/pulsar/client/impl/ClientBuilderImpl.java 80.27% <78.57%> (-0.18%) ⬇️
.../pulsar/client/admin/internal/PulsarAdminImpl.java 80.58% <100.00%> (+3.22%) ⬆️
...lsar/client/impl/conf/ClientConfigurationData.java 96.61% <100.00%> (-0.06%) ⬇️
...a/org/apache/pulsar/client/impl/TableViewImpl.java 61.66% <0.00%> (-24.17%) ⬇️
...va/org/apache/pulsar/common/util/NumberFormat.java 66.66% <0.00%> (-12.50%) ⬇️
.../pulsar/broker/service/SharedConsumerAssignor.java 70.37% <0.00%> (-9.26%) ⬇️
...pache/pulsar/client/impl/TableViewBuilderImpl.java 79.31% <0.00%> (-6.90%) ⬇️
...e/pulsar/common/naming/NamespaceBundleFactory.java 55.55% <0.00%> (-3.89%) ⬇️
... and 73 more

@lhotari lhotari merged commit aa7af10 into apache:master Feb 5, 2023
@michaeljmarshall michaeljmarshall deleted the add-server-cnx-tests branch February 15, 2023 03:00
michaeljmarshall added a commit to michaeljmarshall/pulsar that referenced this pull request Feb 15, 2023
michaeljmarshall added a commit to michaeljmarshall/pulsar that referenced this pull request Feb 15, 2023
michaeljmarshall added a commit to michaeljmarshall/pulsar that referenced this pull request Feb 16, 2023
michaeljmarshall added a commit to michaeljmarshall/pulsar that referenced this pull request Feb 17, 2023
michaeljmarshall added a commit to datastax/pulsar that referenced this pull request Feb 17, 2023
…9430)

(cherry picked from commit aa7af10)
(cherry picked from commit c0f38a0)
(cherry picked from commit 529deaa)
michaeljmarshall added a commit to michaeljmarshall/pulsar that referenced this pull request Feb 17, 2023
…9430)

(cherry picked from commit aa7af10)
(cherry picked from commit c0f38a0)
(cherry picked from commit 0231ad3)
michaeljmarshall added a commit to michaeljmarshall/pulsar that referenced this pull request Feb 17, 2023
…9430)

(cherry picked from commit aa7af10)
(cherry picked from commit c0f38a0)
(cherry picked from commit 0231ad3)
michaeljmarshall added a commit to michaeljmarshall/pulsar that referenced this pull request Feb 22, 2023
…9430)

(cherry picked from commit aa7af10)
(cherry picked from commit c0f38a0)
(cherry picked from commit 0231ad3)
michaeljmarshall added a commit to michaeljmarshall/pulsar that referenced this pull request Feb 23, 2023
…9430)

(cherry picked from commit aa7af10)
(cherry picked from commit c0f38a0)
(cherry picked from commit 0231ad3)
(cherry picked from commit 3e90bc2)
@michaeljmarshall
Copy link
Member Author

I cherry picked this test because it is valid for all release branches and it ensured correctness for some of my cherry picked changes, which is very valuable.

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

Successfully merging this pull request may close these issues.

4 participants