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

Fix SyncAuthorizedEntries test race #4769

Merged
merged 1 commit into from
Jan 3, 2024

Conversation

azdagron
Copy link
Member

@azdagron azdagron commented Jan 2, 2024

The SyncAuthorizedEntries test is set up to optionally send a request, depending on the expectations of the test case. The "no caller ID" and "fetcher fails" test cases are currently set up to send the request on the stream. However, in these instances, the handler exits before reading the request off the stream due to the respective expected failures. Timing conditions still allowed the requests to be sent successfully most of the time but caused intermittent failures, particularly when being run in resource constrained environments (e.g. GH actions).

This change fixes the test cases by not attempting to send the request on the stream.

The SyncAuthorizedEntries test is set up to optionally send a request,
depending on the expectations of the test case. The "no caller ID" and
"fetcher fails" test cases are currently set up to send the request on
the stream. However, in this instances, the handler exits before reading
the request off the stream due to the respective expected failures.
Timing conditions still allowed the requests to be sent successfully
most of the time but caused intermittent failures, particularly when
being run in resource constrained environments (e.g. GH actions).

This change fixes the test cases by not attempting to send the request
on the stream.

Signed-off-by: Andrew Harding <azdagron@gmail.com>
Copy link
Member

@amartinezfayo amartinezfayo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@azdagron azdagron merged commit e1099fe into spiffe:main Jan 3, 2024
32 checks passed
@azdagron azdagron added this to the 1.9.0 milestone Jan 3, 2024
@azdagron azdagron deleted the fix-syncauthorizedentries-test-race branch January 3, 2024 00:51
sriyer pushed a commit to spire-vault/spire that referenced this pull request Feb 23, 2024
The SyncAuthorizedEntries test is set up to optionally send a request,
depending on the expectations of the test case. The "no caller ID" and
"fetcher fails" test cases are currently set up to send the request on
the stream. However, in this instances, the handler exits before reading
the request off the stream due to the respective expected failures.
Timing conditions still allowed the requests to be sent successfully
most of the time but caused intermittent failures, particularly when
being run in resource constrained environments (e.g. GH actions).

This change fixes the test cases by not attempting to send the request
on the stream.

Signed-off-by: Andrew Harding <azdagron@gmail.com>
rushi47 pushed a commit to rushi47/spire that referenced this pull request Apr 11, 2024
The SyncAuthorizedEntries test is set up to optionally send a request,
depending on the expectations of the test case. The "no caller ID" and
"fetcher fails" test cases are currently set up to send the request on
the stream. However, in this instances, the handler exits before reading
the request off the stream due to the respective expected failures.
Timing conditions still allowed the requests to be sent successfully
most of the time but caused intermittent failures, particularly when
being run in resource constrained environments (e.g. GH actions).

This change fixes the test cases by not attempting to send the request
on the stream.

Signed-off-by: Andrew Harding <azdagron@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants