-
Notifications
You must be signed in to change notification settings - Fork 156
test(all): switch to use GitHub strategy matrix and fix flaky tests #828
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
Changes from 21 commits
8ae5f52
6d33329
5426209
4ef4807
583be88
8bfef37
1762f3b
4bf50de
d7661a4
367c90a
4a68307
5eff569
3f0ff65
7290157
1d272d9
dbf3f35
2dde768
1b6e5dc
da44d1b
c62cd4b
bf99b36
bdcf8b8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,48 +2,63 @@ name: run-e2e-tests | |
on: | ||
workflow_dispatch: {} | ||
jobs: | ||
run: | ||
######################### | ||
# Force Github action to run only a single job at a time (based on the group name) | ||
# This is to prevent "race-condition" in building e2e tests infrastructure | ||
######################### | ||
concurrency: | ||
group: e2e-tests | ||
example-and-package-check: | ||
runs-on: ubuntu-latest | ||
permissions: | ||
contents: read | ||
steps: | ||
- name: "Checkout" | ||
uses: actions/checkout@v3 | ||
- name: "Use NodeJS 14" | ||
uses: actions/setup-node@v3 | ||
with: | ||
# Always use version 14 as we use TypeScript target es2020 | ||
node-version: 14 | ||
- name: "Install [email protected]" | ||
run: npm i -g npm@next-8 | ||
- name: "Install monorepo packages" | ||
# This installs all the dependencies of ./packages/* | ||
# See https://github.com/npm/cli/issues/4475 to see why --foreground-scripts | ||
run: npm ci --foreground-scripts | ||
- name: "Install example packages" | ||
# Since we are not managing the cdk examples with npm workspaces we install | ||
# the dependencies in a separate step | ||
working-directory: ./examples/cdk | ||
run: npm ci | ||
- name: "Test packaging" | ||
run: | | ||
npm run lerna-package | ||
cd examples/cdk | ||
npm install ../../packages/**/dist/aws-lambda-powertools-* | ||
npm run test | ||
e2e-tests: | ||
runs-on: ubuntu-latest | ||
permissions: | ||
id-token: write # needed to interact with GitHub's OIDC Token endpoint. | ||
contents: read | ||
strategy: | ||
matrix: | ||
version: [12, 14] | ||
package: [logger, metrics, tracing] | ||
steps: | ||
- name: "Checkout" | ||
uses: actions/checkout@v3 | ||
######################### | ||
# Release new version | ||
######################### | ||
- name: "Use NodeJS 14" | ||
uses: actions/setup-node@v3 | ||
with: | ||
node-version: '14' | ||
- name: Install [email protected] | ||
run: npm i -g npm@next-8 | ||
- name: Install monorepo packages | ||
# This installs all the dependencies of ./packages/* | ||
# See https://github.com/npm/cli/issues/4475 to see why --foreground-scripts | ||
run: npm ci --foreground-scripts | ||
- name: Install example packages | ||
# Since we are not managing the cdk examples with npm workspaces we install | ||
# the dependencies in a separate step | ||
working-directory: ./examples/cdk | ||
run: npm ci | ||
- name: Configure AWS credentials | ||
uses: aws-actions/[email protected] | ||
with: | ||
role-to-assume: ${{ secrets.AWS_ROLE_ARN_TO_ASSUME }} | ||
aws-region: eu-west-1 | ||
- name: Run integration tests | ||
run: npm run lerna-test:e2e | ||
- name: Test packaging | ||
run: | | ||
npm run lerna-package | ||
cd examples/cdk | ||
npm install ../../packages/**/dist/aws-lambda-powertools-* | ||
npm run test | ||
- name: "Checkout" | ||
uses: actions/checkout@v3 | ||
- name: "Use NodeJS 14" | ||
uses: actions/setup-node@v3 | ||
with: | ||
# Always use version 14 as we use TypeScript target es2020 | ||
node-version: 14 | ||
- name: "Install [email protected]" | ||
run: npm i -g npm@next-8 | ||
- name: "Install monorepo packages" | ||
# This installs all the dependencies of ./packages/* | ||
# See https://github.com/npm/cli/issues/4475 to see why --foreground-scripts | ||
run: npm ci --foreground-scripts | ||
- name: "Configure AWS credentials" | ||
uses: aws-actions/[email protected] | ||
with: | ||
role-to-assume: ${{ secrets.AWS_ROLE_ARN_TO_ASSUME }} | ||
aws-region: eu-west-1 | ||
- name: "Run integration tests" | ||
run: | | ||
RUNTIME=nodejs${{ matrix.version }}x npm run test:e2e -w packages/${{ matrix.package }} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
|
||
export const invokeFunction = async (functionName: string, times: number = 1, invocationMode: 'PARALLEL' | 'SEQUENTIAL' = 'PARALLEL', payload: FunctionPayload = {}): Promise<InvocationLogs[]> => { | ||
const invocationLogs: InvocationLogs[] = []; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,6 @@ | ||
export const RESOURCE_NAME_PREFIX = 'Logger-E2E'; | ||
export const TEST_CASE_TIMEOUT = 20_000; // 20 seconds | ||
export const SETUP_TIMEOUT = 300_000; // 300 seconds | ||
export const TEARDOWN_TIMEOUT = 200_000; | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like this syntax. More readable! |
||
export const STACK_OUTPUT_LOG_GROUP = 'LogGroupName'; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,5 @@ | ||
export const RESOURCE_NAME_PREFIX = 'Metrics-E2E'; | ||
export const ONE_MINUTE = 60 * 10_00; | ||
export const TEST_CASE_TIMEOUT = 90_000; // 90 seconds | ||
export const SETUP_TIMEOUT = 300_000; // 300 seconds | ||
export const TEARDOWN_TIMEOUT = 200_000; | ||
export const ONE_MINUTE = 60 * 1000; | ||
export const TEST_CASE_TIMEOUT = 3 * ONE_MINUTE; | ||
export const SETUP_TIMEOUT = 5 * ONE_MINUTE; | ||
export const TEARDOWN_TIMEOUT = 5 * ONE_MINUTE; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
export class FunctionSegmentNotDefinedError extends Error { | ||
public constructor(msg: string) { | ||
super(msg); | ||
Object.setPrototypeOf(this, FunctionSegmentNotDefinedError.prototype); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,11 +14,16 @@ import { | |
expectedCustomResponseValue, | ||
expectedCustomErrorMessage, | ||
} from '../e2e/constants'; | ||
import { FunctionSegmentNotDefinedError } from './FunctionSegmentNotDefinedError'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Question: what do you mean here with segment not defined? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If X-Ray hasn't fully processed all segments, some of them may be missing. |
||
interface ParsedDocument { | ||
name: string | ||
id: string | ||
start_time: number | ||
end_time: number | ||
// 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. By looking at the documentation, also the
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
aws?: { | ||
request_id: string | ||
} | ||
|
@@ -117,6 +122,28 @@ const getTraces = async (xrayClient: XRay, startTime: Date, resourceArn: string, | |
})).sort((a, b) => a.Document.start_time - b.Document.start_time) as ParsedSegment[], | ||
})).sort((a, b) => a.Segments[0].Document.start_time - b.Segments[0].Document.start_time); | ||
|
||
// Verify that all trace has fully loaded invocation subsegments. | ||
// The subsegments may be not available yet or still in progress. | ||
for (const trace of sortedTraces) { | ||
let retryFlag = false; | ||
|
||
let invocationSubsegment; | ||
try { | ||
invocationSubsegment = getInvocationSubsegment(trace); | ||
} catch (error) { | ||
if (error instanceof FunctionSegmentNotDefinedError){ | ||
retry(new Error(`There is no Function subsegment (AWS::Lambda::Function) yet. Retry.`)); | ||
} else { | ||
throw error; | ||
} | ||
} | ||
|
||
retryFlag = retryFlag || (!!invocationSubsegment.in_progress); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Question: essentially here we are looking at the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see! thanks for clarifying |
||
if (retryFlag) { | ||
retry(new Error(`There is at least an invocation subsegment that hasn't been fully processed yet. The "in_progress" flag is still "true" in the document.`)); | ||
} | ||
} | ||
|
||
if (sortedTraces === undefined) { | ||
throw new Error(`Traces are undefined for ${resourceArn}`); | ||
} | ||
|
@@ -139,7 +166,7 @@ const getFunctionSegment = (trace: ParsedTrace): ParsedSegment => { | |
const functionSegment = trace.Segments.find((segment) => segment.Document.origin === 'AWS::Lambda::Function'); | ||
|
||
if (functionSegment === undefined) { | ||
throw new Error('Function segment is undefined'); | ||
throw new FunctionSegmentNotDefinedError('Function segment is undefined. This can be either due to eventual consistency or a bug in Tracer'); | ||
} | ||
|
||
return functionSegment; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Outside of the scope of this PR, but we should rename the tracer folder to "tracer" for consistency. Created an issue:
#829
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#854