test(all): switch to use GitHub strategy matrix and fix flaky tests #828
Conversation
… (and uuid is trimed off)
| @@ -57,7 +57,7 @@ export const createStackWithLambdaFunction = (params: StackWithLambdaFunctionOpt | |||
| }; | |||
|
|
|||
| export const generateUniqueName = (name_prefix: string, uuid: string, runtime: string, testName: string): string => | |||
| `${name_prefix}-${runtime}-${testName}-${uuid}`.substring(0, 64); | |||
| `${name_prefix}-${runtime}-${uuid.substring(0,5)}-${testName}`.substring(0, 64); | |||
There was a problem hiding this comment.
When test name is really long, the uuid is truncated on some resources. This results in name clashing when the same test cases are run at the same time.
dreamorosi
added
the
internal
| strategy: | ||
| matrix: | ||
| version: [12, 14] | ||
| package: [logger, metrics, tracing] |
There was a problem hiding this comment.
Outside of the scope of this PR, but we should rename the tracer folder to "tracer" for consistency. Created an issue:
#829
| export const ONE_MINUTE = 60 * 1000; | ||
| export const TEST_CASE_TIMEOUT = ONE_MINUTE; | ||
| export const SETUP_TIMEOUT = 5 * ONE_MINUTE; | ||
| export const TEARDOWN_TIMEOUT = 5 * ONE_MINUTE; |
There was a problem hiding this comment.
I like this syntax. More readable!
| @@ -14,11 +14,16 @@ import { | |||
| expectedCustomResponseValue, | |||
| expectedCustomErrorMessage, | |||
| } from '../e2e/constants'; | |||
| import { FunctionSegmentNotDefinedError } from './FunctionSegmentNotDefinedError'; | |||
There was a problem hiding this comment.
Question: what do you mean here with segment not defined?
There was a problem hiding this comment.
If X-Ray hasn't fully processed all segments, some of them may be missing.
In our case, we may try to get the Function (AWS::Lambda::Function) segment too early. I use this custom error to distinguish the type of error form others.
| // This flag may be set if the segment hasn't been fully processed | ||
| // The trace may have already appeared in the `getTraceSummaries` response | ||
| // but a segment may still be in_progress | ||
| in_progress?: boolean |
There was a problem hiding this comment.
By looking at the documentation, also the end_time key might be not defined, correct?
end_time – number that is the time the segment was closed. For example, 1480615200.090 or 1.480615200090E9. Specify either an end_time or in_progress.
There was a problem hiding this comment.
Yes. In my experiments, end_time field doesn't exist. It should be optional.
| } | ||
| } | ||
|
|
||
| retryFlag = retryFlag || (!!invocationSubsegment.in_progress); |
There was a problem hiding this comment.
Question: essentially here we are looking at the in_progress key. If it's set, we retry. Correct?
There was a problem hiding this comment.
That's right for this line. But there is a case that the whole segment isn't defined at all. That's handled by the lines above.
There was a problem hiding this comment.
I see! thanks for clarifying
Description of your changes
The e2e tests have been flaky due to following reasons:
error). Causes: We removed the explicit wait on Tracer during refactoring. We now rely on polling until we receive the expected number of traces. However, not all subsegments are available in the traces yet.This PR addresses the issues by:
How to verify this change
You can trigger the e2e test workflow manually.
I've run this 5 times without any failure so far. > https://github.com/awslabs/aws-lambda-powertools-typescript/actions/runs/2295223762.
Related issues, RFCs
#825 <-- the log will be descriptive with Matrix strategy
PR status
Is this ready for review?: YES
Is it a breaking change?: NO
Checklist
Breaking change checklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.