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.

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

test: add CLI timeout #1727

Merged
merged 2 commits into from
Jan 13, 2021
Merged

test: add CLI timeout #1727

merged 2 commits into from
Jan 13, 2021

Conversation

erezrokah
Copy link
Contributor

This PR adds a timeout of 30 seconds for CLI invocations in our tests.

@erezrokah erezrokah requested a review from a team as a code owner January 13, 2021 12:07
@erezrokah erezrokah added the type: chore work needed to keep the product and development running smoothly label Jan 13, 2021
@erezrokah erezrokah requested a review from ehmicky January 13, 2021 13:34
const callCli = async function (args, execOptions) {
const { stdout } = await execa(cliPath, args, { windowsHide: true, windowsVerbatimArguments: true, ...execOptions })
const { stdout } = await execa(cliPath, args, {
windowsHide: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

This is already the default value.
Note: windowsHide is a tricky option which has pros and cons. We probably want to keep the default value here and let Execa decides what the best value for it is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed in 130ee48

const { stdout } = await execa(cliPath, args, { windowsHide: true, windowsVerbatimArguments: true, ...execOptions })
const { stdout } = await execa(cliPath, args, {
windowsHide: true,
windowsVerbatimArguments: true,
Copy link
Contributor

@ehmicky ehmicky Jan 13, 2021

Choose a reason for hiding this comment

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

Is this needed? Would the default behavior might be better at handling Windows?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Don't think it is. Removed in 130ee48

Copy link
Contributor

@ehmicky ehmicky left a comment

Choose a reason for hiding this comment

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

Looks good.
I added two comments which are about options already present in the current code, not changed by this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: chore work needed to keep the product and development running smoothly
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants