s2a tests are order-dependent

[ejona86]

@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.

[NaveenPrasannaV]

@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. Picked changes from PR #11722 and #11731. After all the changes, I ran the GetAuthenticationMechanismsTest class, which is now successful and unable to reproduce the issue.

[ejona86]

@rmehta19, any idea when you might get to this?

[rmehta19]

I opened #11898

I'm sorry about the delay on this. We got this order dependence finding for S2A internally, and I sent the fix then. I just set up email notifications on github, so I can respond to these issues in a timely fashion.