Skip to content

ci: set 30 minutes timeout to the integration test step#1086

Closed
crazy-max wants to merge 1 commit intodocker:mainfrom
crazy-max:ci-test-timeout
Closed

ci: set 30 minutes timeout to the integration test step#1086
crazy-max wants to merge 1 commit intodocker:mainfrom
crazy-max:ci-test-timeout

Conversation

@crazy-max
Copy link
Copy Markdown
Member

follow-up #1085 (comment)

Signed-off-by: CrazyMax <1951866+crazy-max@users.noreply.github.com>
-
name: Test
uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0
timeout-minutes: 30
Copy link
Copy Markdown

@tcely tcely Apr 16, 2026

Choose a reason for hiding this comment

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

Suggested change
timeout-minutes: 30
timeout-minutes: 35

Because the test is already using 30 minutes with test.each, it might be worth lowering the timeout inside the test.

describe('root', () => {
// prettier-ignore
test.each(getSources(true))(
'install docker %s', async (source) => {
await ensureNoSystemContainerd();
const install = new Install({
source: source,
runDir: tmpDir(),
contextName: 'foo',
daemonConfig: `{"debug":true,"features":{"containerd-snapshotter":true}}`
});
await expect(tryInstall(install)).resolves.not.toThrow();
}, 30 * 60 * 1000);
});

const TEST_TIMEOUT = (1000 * 60) * 25; // minutes

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I have asked an AI agent to analyze my proposed change to the timeout values:

By setting the internal test timeout to 25 minutes, you've built in a 10-minute buffer that is critical for two reasons:

  1. The Teardown Window: If a Docker install hangs on macOS, the finally block in your code calls install.tearDown(). Without that 10-minute buffer, the GitHub runner would likely kill the process while it’s still trying to clean up, resulting in lost logs or orphaned resources.
  2. Step Overhead: The GitHub "Test" step isn't just the code execution; it’s also the vitest initialization and the beforeAll hooks. Your 35-minute limit covers that "invisible" time that the 25-minute test timer ignores.

This setup ensures that if things go wrong, the test fails gracefully (with an error message) rather than the runner failing abruptly (with a generic cancellation).

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I will normalize vitest timeouts in relation to runner one

@crazy-max crazy-max closed this Apr 17, 2026
@tcely
Copy link
Copy Markdown

tcely commented Apr 17, 2026

Are you no longer planning for a timeout on this step?

The test does seem to be completing more reliably after your other changes. However, the possibility of hanging as we saw before still exists.

@crazy-max
Copy link
Copy Markdown
Member Author

Are you not longer planning for a timeout on this step?

The test does seem to be completing more reliably after your other changes. However, the possibility of hanging as we saw before still exists.

Plan to do this in vitest instead with global hooks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants