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.

Sign up for GitHub

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

ci: add deploy cleanup step for Vercel tests #2699

Merged
merged 4 commits into from
Oct 29, 2024
Merged

ci: add deploy cleanup step for Vercel tests #2699

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4299ce4
test: add deploy cleanup step for Vercel tests
pieh Oct 25, 2024
b8bedca
test: increase timeout for Vercel tests
pieh Oct 25, 2024
faecd68
test: bump timeout
pieh Oct 28, 2024
729713e
ci: allow using test results from runs triggered by pull_request when…
pieh Oct 28, 2024
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
2 changes: 1 addition & 1 deletion .github/workflows/e2e-report.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ jobs:
id: get-run-id
run: |
if [ "${{ inputs.use-branch }}" == "true" ]; then
E2E_RUN_ID=$(gh run list -w test-e2e.yml -e workflow_dispatch -s success -b $GITHUB_REF_NAME --json databaseId --jq ".[0].databaseId" --repo $GITHUB_REPOSITORY)
E2E_RUN_ID=$(gh run list -w test-e2e.yml -s success -b $GITHUB_REF_NAME --json databaseId --jq ".[0].databaseId" --repo $GITHUB_REPOSITORY)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

not really related to deleting deploys, but this allow to generate test results report from runs on pull request (when label is applied) and not just on workflow_dispatch

else
E2E_RUN_ID=$(gh run list -w test-e2e.yml -e schedule -s success --json databaseId --jq ".[0].databaseId" --repo $GITHUB_REPOSITORY)
fi
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/test-e2e.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ env:
DATADOG_TRACE_NEXTJS_TEST: true
DATADOG_API_KEY: foo
TEST_CONCURRENCY: 2
NEXT_E2E_TEST_TIMEOUT: 300000
NEXT_E2E_TEST_TIMEOUT: 600000
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Similarly as above - not related to deleting deploys, but some of the test suites seemed to be on the verge of timeouts and would sometimes go over so this might reduce some of the flakiness (not very confident in that part due to troubles with reproducing problems locally or even when running individual tests in github actions and not locally, but I think it's worth a try)

NEXT_TELEMETRY_DISABLED: 1
NEXT_SKIP_NATIVE_POSTINSTALL: 1
TURBO_API: ${{ secrets.TURBO_API }}
Expand Down
31 changes: 31 additions & 0 deletions tests/netlify-deploy.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ export class NextDeployInstance extends NextInstance {
private _cliOutput: string
private _buildId: string
private _deployId: string
private _shouldDeleteDeploy: boolean = false

public get buildId() {
// get deployment ID via fetch since we can't access
Expand All @@ -50,6 +51,9 @@ export class NextDeployInstance extends NextInstance {
this._buildId = process.env.BUILD_ID
return
}

const setupStartTime = Date.now()

// create the test site
await super.createTestDir({ parentSpan, skipInstall: true })

Expand Down Expand Up @@ -146,6 +150,7 @@ export class NextDeployInstance extends NextInstance {
this._url = url
this._parsedUrl = new URL(this._url)
this._deployId = deployID
this._shouldDeleteDeploy = !process.env.NEXT_TEST_SKIP_CLEANUP
this._cliOutput = deployRes.stdout + deployRes.stderr
require('console').log(`Deployment URL: ${this._url}`)

Expand All @@ -169,6 +174,32 @@ export class NextDeployInstance extends NextInstance {
).trim()

require('console').log(`Got buildId: ${this._buildId}`)
require('console').log(`Setup time: ${(Date.now() - setupStartTime) / 1000.0}s`)
}

public async destroy(): Promise<void> {
if (this._shouldDeleteDeploy) {
require('console').log(`Deleting project with deploy_id ${this._deployId}`)

const deleteResponse = await execa('npx', [
'ntl',
'api',
'deleteDeploy',
'--data',
`{ "deploy_id": "${this._deployId}" }`,
])

if (deleteResponse.exitCode !== 0) {
require('console').error(
`Failed to delete deploy ${deleteResponse.stdout} ${deleteResponse.stderr} (${deleteResponse.exitCode})`,
)
} else {
require('console').log(`Successfully deleted deploy with deploy_id ${this._deployId}`)
this._shouldDeleteDeploy = false
Copy link
Contributor

Choose a reason for hiding this comment

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

learning - why do we need to set this to false in the end? will it keep attempting and looping if we don't?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should not, this is just in case - I was looking at base class we extend - it does have some checks that look if something was deleted / destroyed already - there they throw:

https://github.com/vercel/next.js/blob/2e28c965279de90ce4bfca673196c27dd6117027/test/lib/next-modes/base.ts#L425-L428

but I didn't want to play with this as I don't fully know the contract that Vercel tests have with those modules - so this just felt safe way to just avoid wasting time on calling API to delete deploy that was already deleted in case this is called multiple times (deleteDeploy api call is not immediate - it takes couple seconds to finish so just no point in calling it again)

}
}

await super.destroy()
}

public get cliOutput() {
Expand Down
Loading