ci(integration-test-deployment): timeout tests before token expiry #36598
Conversation
Abogical
changed the title
github-actions
bot
added
p2
star-contributor
Abogical
force-pushed
the
integ-deployment-timeout-short-circuit-fix
branch
4 times, most recently
from
4a75ce2 to
44e8bb7
Compare
| await allocation.release(outcome); | ||
| try { | ||
| await allocation.release(outcome); | ||
| } catch (e) { |
There was a problem hiding this comment.
Can we catch the token expired error only and not all the errors? Otherwise we're going to hide other problems
There was a problem hiding this comment.
- It already leaves a warning annotation for any errors, including expired tokens.
- Regardless of the error, I don't think that an error when it comes to releasing should be fatal and stop the entire test.
There was a problem hiding this comment.
Regardless of the error, I don't think that an error when it comes to releasing should be fatal and stop the entire test.
I disagree! if we think so then we shouldn't need to release at all. If we think the release is important but we know that there's a specific case isn't important then only catch gracefully this case but not all
There was a problem hiding this comment.
Ok. I've made the error caught only if it is due to an expired security token.
| }); | ||
|
|
||
| test('failed Atmosphere release requests create a warning and proceeds with the next batch', async () => { | ||
| jest.spyOn(integRunner, 'deployIntegrationTest').mockImplementation(() => { |
There was a problem hiding this comment.
What's the point of mocking this part?
There was a problem hiding this comment.
This is to simulate an integration test failure caused by a timeout. This is the same scenario where the original bug occured: https://github.com/aws/aws-cdk/actions/runs/20137124927/job/57793467916#step:12:475
Abogical
force-pushed
the
integ-deployment-timeout-short-circuit-fix
branch
from
44e8bb7 to
4f6af13
Compare
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
Merge Queue Status✅ The pull request has been merged at dfabadf This pull request spent 1 hour 16 minutes 36 seconds in the queue, including 38 minutes 46 seconds running CI. Required conditions to merge
|
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
|
Comments on closed issues and PRs are hard for our team to see. |
2 participants
Reason for this change
Tests that timeout will leave an expired token which can't be used to release Atmosphere resources. See the following for an example: https://github.com/aws/aws-cdk/actions/runs/20137124927/job/57793467916#step:12:475
Description of changes
This commit solves this by throwing a warning instead of an error if the Atmosphere release request fails.
Describe any new or updated permissions being added
No new permissions are added.
Description of how you validated changes
Added unit test.
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license