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.

Sign up for GitHub

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

Jump to bottom

[BUG] Unexpected Coverage Changes on SelfRefreshingKeySet, SecurityInterceptor and SecuritySSLNettyTransport #3137

Closed
4 tasks done
peternied opened this issue Aug 9, 2023 · 5 comments
Closed
4 tasks done

[BUG] Unexpected Coverage Changes on SelfRefreshingKeySet, SecurityInterceptor and SecuritySSLNettyTransport #3137

peternied opened this issue Aug 9, 2023 · 5 comments
Assignees
@peternied @stephen-crawford
Labels
bug Something isn't working flaky-test Flaky Test issue good first issue These are recommended starting points for newcomers looking to make their first contributions. triaged Issues labeled as 'Triaged' have been reviewed and are deemed actionable.

Comments

@peternied
Copy link
Member

peternied commented Aug 9, 2023 •
edited
Loading

What is the bug?
There are several classes that are seeing random behavior in code coverage reports. Test should be added/modified to ensure these code paths are always exercised - keeping codecov results useful.

  • src/main/java/com/amazon/dlic/auth/http/jwt/keybyoidc/SelfRefreshingKeySet.java
    • Has tests to attempt race conditions, might need to alter the class and tests to be able to exercise the 'races' guaranteed
  • src/main/java/org/opensearch/security/transport/SecurityInterceptor.java
    • Needs a tweak, or additional unit test to confirm a local node vs remote node scenario
  • src/main/java/org/opensearch/security/ssl/transport/SecuritySSLNettyTransport.java
    • Needs unit tests
  • src/main/java/org/opensearch/security/configuration/ConfigurationRepository.java
    • Needs unit tests that exercise exceptions around ClusterHealthRequest

image

Codecov [Report link]
Codecov [Unexpected Coverage Changes]

How can one reproduce the bug?

  1. Create a pull request that only changes markdown file
  2. Look at the checks for the pull requset
  3. See the codecov check
  4. See error that coverage has dropped .0X%

What is the expected behavior?
Code coverage should not fluctuate, testing should be deterministic and 'exhaustive'

Do you have any additional context?
This is causing code coverage failures on many pull requests and code is being merged without all checks passing regularly.
@peternied peternied added bug Something isn't working untriaged Require the attention of the repository maintainers and may need to be prioritized good first issue These are recommended starting points for newcomers looking to make their first contributions. flaky-test Flaky Test issue labels Aug 9, 2023
@stephen-crawford
Copy link
Contributor

[Triage] Thank you for filing this issue @peternied. Based on the provided checklist, I will label this issue as actionable and triaged.

@stephen-crawford stephen-crawford added triaged Issues labeled as 'Triaged' have been reviewed and are deemed actionable. and removed untriaged Require the attention of the repository maintainers and may need to be prioritized labels Aug 14, 2023
@peternied peternied mentioned this issue Aug 16, 2023
Merged
3 tasks
This was referenced Aug 23, 2023
Merged
Open
@davidosorno
Copy link
Contributor

@peternied I Started looking into it. Please let me know if you have any useful information that I can use to address this.

@peternied
Copy link
Member Author

@davidosorno I'm excited to see these get fixed, In the descriptions 'checklist' is most of my thoughts - we can dig in more with a pull request - I'd recommend to create a draft, happy to review if you've got questions.

@davidosorno davidosorno mentioned this issue Oct 9, 2023
Merged
3 tasks
@davidosorno
Copy link
Contributor

@peternied I created a draf PR for this issue, please take a look at it and let me know what you think.
Theses classes have a lot of final variables and they're really hard to test, so I hope you can give me an idea about how to improve these tests.

@stephen-crawford stephen-crawford mentioned this issue Jan 4, 2024
Merged
3 tasks
@peternied peternied self-assigned this Jan 16, 2024
@stephen-crawford stephen-crawford self-assigned this Jan 16, 2024
peternied added a commit to peternied/security that referenced this issue Jan 16, 2024
@peternied
Ensure compliance configuration test results are stable
db8aa0a 
There are some cases that use datetime that were causing code coverage
flutuations depending on when the tests are run.

- Related opensearch-project#3137

Signed-off-by: Peter Nied <[email protected]>
peternied added a commit to peternied/security that referenced this issue Jan 16, 2024
@peternied
Ensure compliance configuration test results are stable
efbc58d 
There are some cases that use datetime that were causing code coverage
flutuations depending on when the tests are run, fixed this by adding a
date provider and new unit tests.

- Related opensearch-project#3137

Signed-off-by: Peter Nied <[email protected]>
@peternied peternied mentioned this issue Jan 16, 2024
Merged
3 tasks
@stephen-crawford stephen-crawford mentioned this issue Jan 16, 2024
Merged
3 tasks
@stephen-crawford
Copy link
Contributor

The second and third check boxes should be all set shortly.

DarshitChanpura pushed a commit that referenced this issue Jan 18, 2024
@stephen-crawford
Improve code coverage for SSLNettyTransport class (#3953)
037bc20 
### Description
[Describe what this change achieves]
This change increases code coverage for the SecuritySSLNettyTransport
class. In the middle of 12/23, a few unit tests were added to give
coverage to different parts of the class. This change builds on these
existing changes.

### Issues Resolved
Box three of #3137

Signed-off-by: Stephen Crawford <[email protected]>
dlin2028 pushed a commit to dlin2028/security that referenced this issue May 1, 2024
@stephen-crawford @dlin2028
Improve code coverage for SSLNettyTransport class (opensearch-project…
be1d20f 
…#3953)

### Description
[Describe what this change achieves]
This change increases code coverage for the SecuritySSLNettyTransport
class. In the middle of 12/23, a few unit tests were added to give
coverage to different parts of the class. This change builds on these
existing changes.

### Issues Resolved
Box three of opensearch-project#3137

Signed-off-by: Stephen Crawford <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@peternied peternied

@stephen-crawford stephen-crawford

Labels
bug Something isn't working flaky-test Flaky Test issue good first issue These are recommended starting points for newcomers looking to make their first contributions. triaged Issues labeled as 'Triaged' have been reviewed and are deemed actionable.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@peternied @davidosorno @stephen-crawford