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

s2a tests are order-dependent #11732

Open
ejona86 opened this issue Dec 9, 2024 · 1 comment · May be fixed by #11809
Open

s2a tests are order-dependent #11732

ejona86 opened this issue Dec 9, 2024 · 1 comment · May be fixed by #11809
Labels
code health
Milestone
Next

Comments

@ejona86
Copy link
Member

ejona86 commented Dec 9, 2024

@rmehta19

In #11722 I tried to remove a public API and needed to move a test to a more appropriate package. This changed the test execution order and caused test failures:

GetAuthenticationMechanismsTest > getAuthMechanisms_emptyIdentity_success FAILED
    1 expectation failed:
      1. value of:
             getAuthMechanism(...)
         expected:
             Optional[token: "access_token"
             ]
         but was:
             Optional.empty
                at io.grpc.s2a.internal.handshaker.GetAuthenticationMechanismsTest.getAuthMechanisms_emptyIdentity_success(GetAuthenticationMechanismsTest.java:53)
     

GetAuthenticationMechanismsTest > getAuthMechanisms_nonEmptyIdentity_success FAILED
    1 expectation failed:
      1. value of:
             getAuthMechanism(...)
         expected:
             Optional[identity {
               spiffe_id: "fake-spiffe-id"
             }
             token: "access_token"
             ]
         but was:
             Optional.empty
                at io.grpc.s2a.internal.handshaker.GetAuthenticationMechanismsTest.getAuthMechanisms_nonEmptyIdentity_success(GetAuthenticationMechanismsTest.java:62)

In investigating the failure, I found multiple global states that were mutated by tests and not restored. Those are being fixed in #11731. But I also saw that GetAuthenticationMechanisms will copy some of the global state and remember it. Thus even after restoring the mutated state the tests are still order-dependent.

It isn't at all clear to me what the point of GetAuthenticationMechanisms is, as it seems to be a very convoluted approach to read an environment variable. I don't understand why TokenFetcher receives an S2AIdentity, as the only implementation ignores it. I suspect some of this may be for testing, although it seems dependency injection would be much better suited.
@ejona86 ejona86 added this to the Next milestone Dec 9, 2024
@ejona86 ejona86 mentioned this issue Dec 9, 2024
Open
@NaveenPrasannaV NaveenPrasannaV linked a pull request Jan 8, 2025 that will close this issue
Draft
@NaveenPrasannaV
Copy link
Contributor

NaveenPrasannaV commented Jan 8, 2025 •
edited
Loading

@ejona86 @shivaspeaks
For s2a tests are order-dependent, my analysis was to move the test to a better package, fix global state issues, simplify GetAuthenticationMechanisms to read the environment variable directly, and refactor TokenFetcher to remove S2AIdentity and use dependency injection. To achieve this I moved the classes to a more appropriate package as per #11722 and made changes to the files according to #11731. After all the changes, I ran the GetAuthenticationMechanismsTest class, which is now successful and unable to reproduce the issue.
11732-GetAuthenticationMechanismsTest

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
code health
Projects
None yet
Milestone
Next
Development

Successfully merging a pull request may close this issue.

s2a: refactored all the classes into a package NaveenPrasannaV/grpc-java
2 participants
@ejona86 @NaveenPrasannaV