Re-enable hash tests after unsuccessfully trying to repro.

[absoludity]

Perhaps CI will repro the bug for us.

Signed-off-by: Michael Nelson [email protected]

Description of the change

I'd created #4900 in June after noticing occasional failures, but then not seeing those in CI at all, so we didn't land the PR until Antonio noted the errors occurring again nearly 3 months later.

I've just tried to reproduce the transient failure in a few different comments (now and then), unsuccessfully, so am not sure how we can move forward other than re-enabling for now in the hope that CI may trigger the issue, if it still exists.

The issue was that part of the TokenCredentialRequest that is generated for the test depends on an env variable (specifically, DEFAULT_PINNIPED_API_SUFFIX) which was also set in another test test_get_api_group_getters. Rust runs tests concurrently, so if the test happened to be run in a thread of the same process before the hash test, then they would generate a different (but predictable) hash. The actual different hash can be reproduced simply by setting the env var manually to the same value set in the test:

$ DEFAULT_PINNIPED_API_SUFFIX="foo.bar" cargo test test_token_credential
    Finished test [unoptimized + debuginfo] target(s) in 0.12s
     Running unittests src/main.rs (target/debug/deps/pinniped_proxy-377bbebe777ff780)

running 8 tests
test pinniped::tests::test_token_credential_request_hash_differs_with_authenticator ... ok
test pinniped::tests::test_token_credential_request_hash_differs_with_token ... ok
test pinniped::tests::test_token_credential_request_with_future_expiry_is_not_expired ... ok
test pinniped::tests::test_token_credential_request_with_old_expiry_is_expired ... ok
test pinniped::tests::test_token_credential_request_without_credential_is_expired ... ok
test pinniped::tests::test_token_credential_request_hash_default ... FAILED
test pinniped::tests::test_token_credential_request_hash_identical_with_status_change ... FAILED
test pinniped::tests::test_token_credential_request_without_status_is_expired ... ok

failures:

---- pinniped::tests::test_token_credential_request_hash_default stdout ----
thread 'pinniped::tests::test_token_credential_request_hash_default' panicked at 'assertion failed: `(left == right)`
  left: `2671791248625081931`,
 right: `2363471629413450951`', src/pinniped.rs:549:9

---- pinniped::tests::test_token_credential_request_hash_identical_with_status_change stdout ----
thread 'pinniped::tests::test_token_credential_request_hash_identical_with_status_change' panicked at 'assertion failed: `(left == right)`
  left: `2671791248625081931`,
 right: `2363471629413450951`', src/pinniped.rs:593:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace


failures:
    pinniped::tests::test_token_credential_request_hash_default
    pinniped::tests::test_token_credential_request_hash_identical_with_status_change

test result: FAILED. 6 passed; 2 failed; 0 ignored; 0 measured; 22 filtered out; finished in 0.00s

The solution is to ensure that the tests which modify the environment restore the environment after completing, and that any test which depends on those environment variables run with a mutex (so they don't run concurrently). Hence using the temp-env crate which can be used to ensure just that.

Benefits

No more transient failures for these tests.

Possible drawbacks

None

Applicable issues

• Fixes Invesitgate transient errors in pinniped-proxy hash-related tests #5296

Additional information

[netlify]

✅ Deploy Preview for kubeapps-dev canceled.

Built without sensitive environment variables

Name	Link
🔨 Latest commit	1b29066
🔍 Latest deploy log	https://app.netlify.com/sites/kubeapps-dev/deploys/6375bb3fd48ae700087d3fdc

[absoludity]

Perhaps CI will repro the bug for us.

Woo - CI for the win:

failures:

---- pinniped::tests::test_token_credential_request_hash_default stdout ----
thread 'pinniped::tests::test_token_credential_request_hash_default' panicked at 'assertion failed: `(left == right)`
  left: `2671791248625081931`,
 right: `2363471629413450951`', src/pinniped.rs:549:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

---- pinniped::tests::test_token_credential_request_hash_identical_with_status_change stdout ----
thread 'pinniped::tests::test_token_credential_request_hash_identical_with_status_change' panicked at 'assertion failed: `(left == right)`
  left: `2671791248625081931`,
 right: `2363471629413450951`', src/pinniped.rs:593:9


failures:
    pinniped::tests::test_token_credential_request_hash_default
    pinniped::tests::test_token_credential_request_hash_identical_with_status_change

test result: FAILED. 28 passed; 2 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.09s

[antgamdia]

Thanks for the fix and for the explanation... concurrency seems to be always the root cause of most of the "transient errors", haha. +1, but please do not merge the PR until we trigger the release, otherwise, the OSL file will get out of sync.

[antgamdia]

As we are waiting for the OSM team to provide the licenses file, we need to hold this change back for now. Please, do not merge it until the release 2.6.1 has been triggered 🙏

[absoludity]

Given that this is a dev-dependency only, I wonder if we can update the licenses file to exclude dev-only dependencies? (In that, nothing about it is being released).

[absoludity]

Thanks for the fix and for the explanation... concurrency seems to be always the root cause of most of the "transient errors", haha.

Heh - yeah, though I'd be more specific and blame concurrent writes of a global variable (in this case an environment variable). Concurrency should become the norm, but our programming needs to update. Ironically, Rust does a huge amount to stop you accessing actual language variables concurrently for writes etc., but it can't do much when we're updating an OS-level variable in different places.

+1, but please do not merge the PR until we trigger the release, otherwise, the OSL file will get out of sync.

Yep, ok. I'll wait until the release is triggered etc. Thanks Antonio.