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

s2a tests are order-dependent #11732

Open
ejona86 opened this issue Dec 9, 2024 · 0 comments
Open

s2a tests are order-dependent #11732

ejona86 opened this issue Dec 9, 2024 · 0 comments
Milestone

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.

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

No branches or pull requests

1 participant