Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
135 changes: 134 additions & 1 deletion .github/workflows/test_e2e_portal.yml
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,11 @@ name: 'Test: E2E (Portal)'

on:
workflow_dispatch:
inputs:
repeat_count:
description: 'Number of repetitions per test when hunting flaky tests'
type: string
default: '30'
Comment thread
nielsVoogt marked this conversation as resolved.
merge_group:
types: [checks_requested]
pull_request:
Expand Down Expand Up @@ -81,6 +86,127 @@ jobs:
working-directory: ${{ env.e2eTestsPath }}
run: 'npm run lint'

detect-flaky-candidates:
needs: path-filter

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this can/should wait on the linting at least to succeed...

runs-on: ubuntu-latest
if: ${{ needs.path-filter.outputs.should_skip != 'true' && (github.event_name == 'pull_request' || github.event_name == 'workflow_dispatch') }}
outputs:
files: ${{ steps.select.outputs.files }}
has_candidates: ${{ steps.select.outputs.has_candidates }}
steps:
- name: Check out repository code
uses: actions/checkout@v7
with:
filter: blob:none # Reduce clone size/speed
fetch-depth: 0 # Full history is needed to diff against the base commit

- name: Set up Node.js version
uses: actions/setup-node@v7
with:
node-version-file: '${{ env.e2eTestsPath }}/.node-version'

- name: Determine base commit for flaky-candidate selection
id: base
run: |
if [ "${{ github.event_name }}" = "pull_request" ]; then
base_ref="${{ github.event.pull_request.base.ref }}"
else
# Manual runs have no PR base, so diff against the default branch instead.
base_ref="${{ github.event.repository.default_branch }}"
fi
# Merge-base (not the branch tip) avoids treating base-branch commits
# made after this branch diverged as changes introduced by this PR.
echo "sha=$(git merge-base "origin/$base_ref" HEAD)" >> "$GITHUB_OUTPUT"

- name: Select changed/new E2E spec files for flaky-test detection
id: select
run: |
node tools/select-changed-e2e-tests.mjs --base ${{ steps.base.outputs.sha }} --head ${{ github.sha }}

find-flaky-changed-tests:
runs-on: ubuntu-latest
needs:
- path-filter
- lint-code
- detect-flaky-candidates
Comment on lines +128 to +131

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
needs:
- path-filter
- lint-code
- detect-flaky-candidates
needs: detect-flaky-candidates

This would only need to mention its 'critical path'-dependencies, I'd think... Not "the whole dependency tree"...

if: ${{ needs.detect-flaky-candidates.outputs.has_candidates == 'true' }}
strategy:
fail-fast: false
matrix:
file: ${{ fromJson(needs.detect-flaky-candidates.outputs.files) }}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This would spawn a runner/job per-changed-spec-file, right?

I wonder how we can 'constrain' this, within some sane limits... 🤔


steps:
- name: Check out repository code
uses: actions/checkout@v7

- name: Set up Node.js version
uses: actions/setup-node@v7
with:
node-version-file: '${{ env.e2eTestsPath }}/.node-version'
cache: 'npm'
cache-dependency-path: '${{ env.e2eTestsPath }}/package-lock.json'

- name: Install E2E-Tests code-dependencies
working-directory: ${{ env.e2eTestsPath }}
# We only need to install the tests' dependencies, so we can skip the `postinstall`-step
run: |
npm ci --ignore-scripts

- name: Set ENV-variables for test-environment
run: |
cp services/.env.example services/.env

- name: Run Services with Docker
run: |
npm run start:services:ci:production

- name: Run Portal
working-directory: ./interfaces/portal
env:
NG_PRODUCTION: true
NG_URL_121_SERVICE_API: http://localhost:3000/api
NG_DOWNLOAD_TRANSLATIONS_AT_BUILD: false
run: |
npm ci
cp .env.example .env
npm run start:debug-production > run-portal-logs.txt 2>&1 &

- name: Install E2E-Tests runtime-dependencies
working-directory: ${{ env.e2eTestsPath }}
run: |
npx playwright install chromium --with-deps --no-shell

- name: Wait for Portal
run: |
timeout 90s sh -c 'until curl http://localhost:8088 --head --silent; do echo "Waiting for Portal to be running..."; sleep 1; done'
Comment on lines +139 to +181

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It feels really stupid that we'd need to duplicate this whole chain here again...

We could wrap it up into a 'composite action', maybe... But that would only deduplicate the written code... It should also somehow de-duplicate the work... 🤔


- name: Hunt for flaky tests in changed/new spec file

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
- name: Hunt for flaky tests in changed/new spec file
- name: Find flaky tests in changed/new E2E spec file(s)

working-directory: ${{ env.e2eTestsPath }}
env:
# Passed via env (not inlined into the script) since SPEC_FILE originates from PR file paths and must never be interpreted by the shell.
SPEC_FILE: ${{ matrix.file }}
REPEAT_COUNT: ${{ github.event.inputs.repeat_count || '30' }}
run: |
npm test -- "$SPEC_FILE" --fail-on-flaky-tests --repeat-each "$REPEAT_COUNT" -x --trace retain-on-failure --reporter=html

- uses: actions/upload-artifact@v7
if: always()
with:
name: test-result-artifacts-flaky-${{ strategy.job-index }}
path: |
${{ env.e2eTestsPath }}/test-results/
${{ env.e2eTestsPath }}/playwright-report/
./interfaces/portal/run-portal-logs.txt

- name: Portal logs
if: always()
shell: bash
run: cat ./interfaces/portal/run-portal-logs.txt

- name: Docker logs
if: always()
uses: global-121/collect-container-logs@78594c6154439e32858bf3dc17c9ff36561a1d31

test-shard-e2e:
runs-on: ubuntu-latest
needs:
Expand Down Expand Up @@ -165,7 +291,14 @@ jobs:
# This is a workaround for this issue: https://github.com/orgs/community/discussions/44490
test-shard-resolution-e2e:
runs-on: ubuntu-slim
needs: [test-shard-e2e]
needs:
[
path-filter,
lint-code,
detect-flaky-candidates,
test-shard-e2e,
find-flaky-changed-tests,
]
Comment on lines +295 to +301

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
[
path-filter,
lint-code,
detect-flaky-candidates,
test-shard-e2e,
find-flaky-changed-tests,
]
- path-filter
- lint-code
- detect-flaky-candidates
- test-shard-e2e
- find-flaky-changed-tests

'Plain' YAML syntax is good enough... the [...]-syntax is just a hack to get it on 1 line...

if: always()
steps:
- name: Check required job results
Expand Down
3 changes: 2 additions & 1 deletion tools/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,8 @@
"check-versions": "node check-versions.mjs",
"download-logs": "node --env-file-if-exists=.env download-logs.mjs",
"find-flaky-tests-API": "node find-flaky-tests-API.mjs",
"find-flaky-tests-E2E": "node find-flaky-tests-E2E.mjs"
"find-flaky-tests-E2E": "node find-flaky-tests-E2E.mjs",
"select-changed-e2e-tests": "node select-changed-e2e-tests.mjs"
},
"dependencies": {
"@azure/identity": "^4.13.1",
Expand Down
187 changes: 187 additions & 0 deletions tools/select-changed-e2e-tests.mjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,187 @@
#!/usr/bin/env node

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think we should be starting to add some linting to this tools-folder...

With adding more and more code, it should all "look alike" and be checked for recommended/best-practice issues etc... (like the MJS-code in the interfaces/portal/scripts-folder..)

And there are probably some more utility-like methods that can be shared around more broadly...


/**
* Selects Playwright E2E spec files that changed "enough" in a pull request
* to be worth running through the flaky-test hunter before merging:
* - Brand-new spec files (didn't exist in the base commit) are always selected.
* - Existing spec files are selected if the changed lines (added + deleted)
* make up at least `--threshold` percent of the file's current line count.
*
* Used by .github/workflows/test_e2e_portal.yml to decide which spec files
* (if any) to run via `find-flaky-changed-tests`.
*
* Usage:
* node select-changed-e2e-tests.mjs --base <sha> [--head HEAD]
* [--threshold 10] [--max-files 15]
*/
import { execFile } from 'node:child_process';
import { appendFileSync, readFileSync } from 'node:fs';
import { parseArgs } from 'node:util';

const execFileAsync = ({ command, commandArgs, options = {} }) =>
new Promise((resolve, reject) => {
execFile(command, commandArgs, options, (error, stdout) => {
if (error) {
reject(error);
return;
}
resolve(stdout);
});
});
Comment on lines +19 to +30

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this could be replaces by:

Suggested change
import { parseArgs } from 'node:util';
const execFileAsync = ({ command, commandArgs, options = {} }) =>
new Promise((resolve, reject) => {
execFile(command, commandArgs, options, (error, stdout) => {
if (error) {
reject(error);
return;
}
resolve(stdout);
});
});
import { parseArgs, promisify } from 'node:util';
const execFileAsync = promisify(execFile);


const E2E_SPEC_TESTS_DIRECTORY = 'e2e/portal/tests/';

const { values: args } = parseArgs({
options: {
base: { type: 'string' },
head: { type: 'string', default: 'HEAD' },
threshold: { type: 'string', default: '10' },
'max-files': { type: 'string', default: '15' },

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
'max-files': { type: 'string', default: '15' },
base: { type: 'string' },
head: {
type: 'string',
default: 'HEAD'
},
threshold: {
type: 'string',
default: '10'
},
'max-files': {
type: 'string',
default: '15'
},

Would be a bit more readable...

},
});

if (!args.base) {
console.error('Missing required argument: --base <sha>');
process.exit(1);
}

const threshold = Number(args.threshold);
// Bounds how many parallel Docker+Portal CI jobs a single PR can trigger.
const maxFiles = Number(args['max-files']);

async function getRepositoryRoot() {
const stdout = await execFileAsync({
command: 'git',
commandArgs: ['rev-parse', '--show-toplevel'],
});
return stdout.trim();
}

async function getChangedSpecFiles({ repositoryRoot, base, head }) {
const stdout = await execFileAsync({
command: 'git',
commandArgs: [
'diff',
'--no-renames',
'--diff-filter=AM', // Excludes deleted paths, which no longer exist in `head` to read.
'--numstat',
base,
head,
],
options: { cwd: repositoryRoot },
});

return stdout
.split('\n')
.filter((line) => line.trim() !== '')
.map((line) => {
const [added, deleted, path] = line.split('\t');
return { added, deleted, path };
})
.filter(
({ added, deleted, path }) =>
added !== '-' && // Excludes binary files, which have no line-based diff.
deleted !== '-' &&
path.startsWith(E2E_SPEC_TESTS_DIRECTORY) &&
path.endsWith('.spec.ts'),
);
}

async function isNewFile({ repositoryRoot, base, path }) {
try {
await execFileAsync({

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Maybe we don't need to make something-sync-async-and-then-await it?
Why not "just run the command sync"?

command: 'git',
commandArgs: ['cat-file', '-e', `${base}:${path}`],
options: { cwd: repositoryRoot },
});
return false;
} catch {
return true;
}
}

function getChangePercentage({ repositoryRoot, path, added, deleted }) {
const content = readFileSync(`${repositoryRoot}/${path}`, 'utf8');
const totalLines = content.split('\n').length;
if (totalLines === 0) {
return 100;
}

const changedLines = Number(added) + Number(deleted);
return (changedLines / totalLines) * 100;
}

async function selectCandidateSpecFiles({ repositoryRoot, base, head }) {
const changedSpecFiles = await getChangedSpecFiles({
repositoryRoot,
base,
head,
});

const candidates = [];
for (const { added, deleted, path } of changedSpecFiles) {
const isNew = await isNewFile({ repositoryRoot, base, path });
if (isNew) {
candidates.push({ path, reason: 'new file' });
continue;
}

const percentage = getChangePercentage({
repositoryRoot,
path,
added,
deleted,
});
if (percentage >= threshold) {
candidates.push({ path, reason: `${percentage.toFixed(1)}% changed` });
}
}

return candidates;
}

function capCandidates({ candidates }) {
if (candidates.length <= maxFiles) {
return candidates;
}

console.warn(
`Found ${candidates.length} candidate spec files, which exceeds --max-files (${maxFiles}). Only the first ${maxFiles} will be checked for flakiness.`,
);
return candidates.slice(0, maxFiles);
}

function toE2eRelativePath({ path }) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Weird capitalization...

Maybe just "toRelativePath"?

return path.slice('e2e/'.length);
}

function writeGithubOutput({ candidates }) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Weird capitalization, it's "GitHub", so: writeGitHubOutput.

const files = candidates.map(({ path }) => toE2eRelativePath({ path }));
const hasCandidates = files.length > 0;

const outputPath = process.env.GITHUB_OUTPUT;
if (outputPath) {
appendFileSync(outputPath, `files=${JSON.stringify(files)}\n`);
appendFileSync(outputPath, `has_candidates=${hasCandidates}\n`);
}
}

const repositoryRoot = await getRepositoryRoot();
const candidates = capCandidates({
candidates: await selectCandidateSpecFiles({
repositoryRoot,
base: args.base,
head: args.head,
}),
});

if (candidates.length === 0) {
console.log('No changed/new E2E spec files meet the threshold.');
} else {
console.log('Selected E2E spec files for flaky-test detection:');
for (const { path, reason } of candidates) {
console.log(` - ${path} (${reason})`);
}
}

writeGithubOutput({ candidates });
Loading