Enhance RestLayerPrivilegesEvaluator Code Coverage #3218

stephen-crawford · 2023-08-21T14:07:34Z

Description

RestLayerPrivilegesEvaluator is expected to have 100% code coverage. This change should boost the coverage as high as is reasonable. Per this comment: #2929 (comment), I am not adding tests for debug logs.

Issues Resolved

Resolves #2929

Check List

New functionality includes testing
~~New functionality has been documented~~
Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Stephen Crawford <steecraw@amazon.com>

peternied · 2023-08-21T15:28:34Z

src/test/java/org/opensearch/security/filter/RestPathMatchesTest.java

If code cov says that there is a no change, are you sure these tests are executed and their coverage data submitted? Do you see these tests executed from the artifacts of the check runs?

It does not look like it ran. I am not sure how to fix that though. I don't think it is something related to this change--I triggered a re-run.

@scrawfor99 I think you need to call it RestPathMatchesTests.java not RestPathMatchesTest.java.

Maybe it is not only your case but others as well.

Nice catch @willyborankin . I've got a change that will expect 100% code coverage in the tests (e.g. all test code should be executed) that should help us catch this kind of issue

Add code coverage exclusions on false positives #3196

Yes I did the same mistake :-D. Here is PR: #3224. Lets see.

Huh, who knew. I will swap over the name.

Signed-off-by: Stephen Crawford <steecraw@amazon.com>

stephen-crawford · 2023-08-22T14:15:23Z

Confirmed tests ran.

stephen-crawford · 2023-08-22T14:18:25Z

RestLayerPrivilegeEvaluator still lists isInitialized as having partial coverage. However, I think this is adequate. I added a test to check that it fails when not initialized (one of the two possibilities) and all other tests which test something else will inherently check isInitialized is true during their evaluation. This covers the true case.

stephen-crawford · 2023-08-22T14:22:32Z

This is the code coverage for the other SecurityRestFilter: https://app.codecov.io/gh/opensearch-project/security/pull/3218/blob/src/main/java/org/opensearch/security/filter/SecurityRestFilter.java

It still labels a significant amount as uncovered. I am not sure what is expected here however, as the way to exercise the files is to extract the internal method logic into separate functions so we can provide good and bad inputs and test the handling.

In RestLayerPrivilegesEvaluator there was a number of unused parameters removed those to trim down the test requirements. For the test cases switched to assertThat for easy of reading, enabled debug logging to exercise debug logging code, adjusted usages of mocks for cleaner test flow in a couple of spaces. Verified locally with the following command and then checked the jacoco report under build/reports/jacoco/test/html/... ./gradlew jacocoTestReport test --tests \ org.opensearch.security.privileges.RestLayerPrivilegesEvaluatorTest Signed-off-by: Peter Nied <petern@amazon.com>

…erage Signed-off-by: Peter Nied <petern@amazon.com>

peternied

I've created a pull request for your fork that reduces the product code, adds more assertions, and gets more coverage, take a look and see if you'd like to include it with PR

Increase coverage target stephen-crawford/security#4

Increase coverage target

codecov · 2023-08-22T17:12:03Z

Codecov Report

Merging #3218 (b58e3bc) into main (75f529a) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##               main    #3218      +/-   ##
============================================
+ Coverage     62.43%   62.45%   +0.01%     
+ Complexity     3352     3351       -1     
============================================
  Files           254      254              
  Lines         19748    19743       -5     
  Branches       3334     3334              
============================================
  Hits          12330    12330              
+ Misses         5789     5785       -4     
+ Partials       1629     1628       -1

Files Changed	Coverage Δ
.../opensearch/security/OpenSearchSecurityPlugin.java	`84.34% <100.00%> (ø)`
...urity/privileges/RestLayerPrivilegesEvaluator.java	`94.11% <100.00%> (+12.06%)`	⬆️

src/test/java/org/opensearch/security/privileges/RestLayerPrivilegesEvaluatorTest.java

Signed-off-by: Stephen Crawford <steecraw@amazon.com>

src/test/java/org/opensearch/security/privileges/RestLayerPrivilegesEvaluatorTest.java

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

Cleans up RestLayerPrivilegesEvaluatorTest

DarshitChanpura

LGTM. Thanks @scrawfor99 !

opensearch-trigger-bot · 2023-08-24T16:44:38Z

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/security/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/security/backport-2.x
# Create a new branch
git switch --create backport/backport-3218-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 46dfd841abeae193075d8cf95dd4023e9ea71ef9
# Push it to GitHub
git push --set-upstream origin backport/backport-3218-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/security/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-3218-to-2.x.

…t#3218) Enhance RestLayerPrivilegesEvaluator Code Coverage Signed-off-by: Stephen Crawford <steecraw@amazon.com> Signed-off-by: Peter Nied <petern@amazon.com> Signed-off-by: Darshit Chanpura <dchanp@amazon.com> Co-authored-by: Peter Nied <petern@amazon.com> Co-authored-by: Darshit Chanpura <dchanp@amazon.com> (cherry picked from commit 46dfd84)

DarshitChanpura · 2023-08-24T16:49:33Z

Backported in #2956 as that is the parent change for this PR.

stephen-crawford and others added 4 commits August 21, 2023 10:02

PathMatches

64bdf87

Signed-off-by: Stephen Crawford <steecraw@amazon.com>

coverage

8efb311

Signed-off-by: Stephen Crawford <steecraw@amazon.com>

Spotless

d4d15ec

Signed-off-by: Stephen Crawford <steecraw@amazon.com>

Merge branch 'opensearch-project:main' into boostedCoverage

e1b6c7b

stephen-crawford requested review from cliu123, cwperks, DarshitChanpura, davidlago, peternied, RyanL1997, reta and willyborankin as code owners August 21, 2023 14:07

willyborankin previously approved these changes Aug 21, 2023

View reviewed changes

peternied reviewed Aug 21, 2023

View reviewed changes

stephen-crawford and others added 3 commits August 21, 2023 13:07

Merge branch 'opensearch-project:main' into boostedCoverage

b46f25c

Merge branch 'opensearch-project:main' into boostedCoverage

f9756f3

Fix naming

acb23d2

Signed-off-by: Stephen Crawford <steecraw@amazon.com>

stephen-crawford dismissed willyborankin’s stale review via acb23d2 August 22, 2023 13:41

peternied added 2 commits August 22, 2023 11:35

Merge remote-tracking branch 'steven/boostedCoverage' into boostedCov…

b1ac98b

…erage Signed-off-by: Peter Nied <petern@amazon.com>

peternied reviewed Aug 22, 2023

View reviewed changes

DarshitChanpura previously approved these changes Aug 22, 2023

View reviewed changes

Merge pull request #4 from peternied/boostedCoverage

3fb77a7

Increase coverage target

stephen-crawford dismissed DarshitChanpura’s stale review via 3fb77a7 August 22, 2023 17:07

peternied reviewed Aug 22, 2023

View reviewed changes

src/test/java/org/opensearch/security/privileges/RestLayerPrivilegesEvaluatorTest.java Show resolved Hide resolved

Fix mock

e408e40

Signed-off-by: Stephen Crawford <steecraw@amazon.com>

DarshitChanpura reviewed Aug 23, 2023

View reviewed changes

src/test/java/org/opensearch/security/privileges/RestLayerPrivilegesEvaluatorTest.java Outdated Show resolved Hide resolved

DarshitChanpura and others added 2 commits August 23, 2023 11:53

Cleans up RestLayerPrivilegesEvaluatorTest

c762490

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

Merge pull request #5 from DarshitChanpura/cleanup-restevaluatortest

b58e3bc

Cleans up RestLayerPrivilegesEvaluatorTest

DarshitChanpura approved these changes Aug 23, 2023

View reviewed changes

peternied approved these changes Aug 23, 2023

View reviewed changes

peternied merged commit 46dfd84 into opensearch-project:main Aug 23, 2023

DarshitChanpura added the backport 2.x backport to 2.x branch label Aug 24, 2023

opensearch-trigger-bot bot added the backport-failed label Aug 24, 2023

peternied removed backport 2.x backport to 2.x branch backport-failed labels Aug 31, 2023

stephen-crawford deleted the boostedCoverage branch September 14, 2023 14:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhance RestLayerPrivilegesEvaluator Code Coverage #3218

Enhance RestLayerPrivilegesEvaluator Code Coverage #3218

stephen-crawford commented Aug 21, 2023

peternied Aug 21, 2023

stephen-crawford Aug 21, 2023

willyborankin Aug 21, 2023 •

edited

Loading

willyborankin Aug 21, 2023

peternied Aug 21, 2023

willyborankin Aug 21, 2023

stephen-crawford Aug 22, 2023

stephen-crawford commented Aug 22, 2023

stephen-crawford commented Aug 22, 2023

stephen-crawford commented Aug 22, 2023 •

edited

Loading

peternied left a comment

codecov bot commented Aug 22, 2023 •

edited

Loading

DarshitChanpura left a comment

opensearch-trigger-bot bot commented Aug 24, 2023

DarshitChanpura commented Aug 24, 2023

Enhance RestLayerPrivilegesEvaluator Code Coverage #3218

Enhance RestLayerPrivilegesEvaluator Code Coverage #3218

Conversation

stephen-crawford commented Aug 21, 2023

Description

Issues Resolved

Check List

peternied Aug 21, 2023

Choose a reason for hiding this comment

stephen-crawford Aug 21, 2023

Choose a reason for hiding this comment

willyborankin Aug 21, 2023 • edited Loading

Choose a reason for hiding this comment

willyborankin Aug 21, 2023

Choose a reason for hiding this comment

peternied Aug 21, 2023

Choose a reason for hiding this comment

willyborankin Aug 21, 2023

Choose a reason for hiding this comment

stephen-crawford Aug 22, 2023

Choose a reason for hiding this comment

stephen-crawford commented Aug 22, 2023

stephen-crawford commented Aug 22, 2023

stephen-crawford commented Aug 22, 2023 • edited Loading

peternied left a comment

Choose a reason for hiding this comment

codecov bot commented Aug 22, 2023 • edited Loading

Codecov Report

DarshitChanpura left a comment

Choose a reason for hiding this comment

opensearch-trigger-bot bot commented Aug 24, 2023

DarshitChanpura commented Aug 24, 2023

willyborankin Aug 21, 2023 •

edited

Loading

stephen-crawford commented Aug 22, 2023 •

edited

Loading

codecov bot commented Aug 22, 2023 •

edited

Loading