Skip to content

Conversation

cderv
Copy link
Collaborator

@cderv cderv commented Oct 30, 2024

This is a follow up on 00249a4 which closed #10442

It adds a test from the error message when environment variable is set.

To be working it required to mock Deno.exit() in the test so that it throws an Error()

This is because after using error(), Deno.exit(1) is called, and it makes test.execute() quite with the rest of the test command to verify assertion.

As a side note, this is possibly a problem as anytime this Deno.exit() happens, our test suite won't fail ! Test is not seen as Failed nor Success 🤷‍♂️

Our usual pattern seem to be throwing an error after error() so that our hooks trigger (the stack trace for example)

Some examples:

error(
`${src} uses server: shiny so cannot be included in a website project ` +
`(shiny documents require a backend server and so can't be published as static web content).`,
);
throw new Error();

error(
`${src} is a knitr engine document that uses server: shiny so cannot be included in a project with an output-dir ` +
`(shiny document output must be rendered alongside its source document).`,
);
throw new Error();

if (!result.success) {
error("Error running Pandoc: " + result.stderr);
throw new Error(result.stderr);
}

Though it is not the case everywhere. 🤷‍♂️ So probably not applying in this checking of quarto version requirement where we want to fail fast.

Using Deno.exit(1) completely stop execution and catch() does not apply

quarto-cli/tests/test.ts

Lines 176 to 180 in 60b8e01

try {
await test.execute();
} catch (e) {
logError(e);
}

Anyhow, just a PR so that we have a test on the feature; Hopefully this is a good way to do it in our current test status.

cderv added 2 commits October 30, 2024 12:05
This is required as Deno.exit(1) after `error()` is making `test.execute()` exit without possibility to cath the error and log the error to be verified.

Our test logic does not even fail on this Deno.exit() happening. Tests continue and nor OK or FAILED is thrown.

Our usual pattern to throw an error when using error(), but it may not apply in this case for checking version requirement in quarto.ts. This is why mocking is chosen here.
@cderv cderv requested a review from cscheid October 30, 2024 12:08
@cscheid
Copy link
Collaborator

cscheid commented Oct 30, 2024

This works.

I think, ideally, we'd hide all calls to Deno.exit behind a function in deno_ral, so that this kind of mocking is easier and doesn't require referring to Deno entry points at all.

@cscheid cscheid merged commit a0c23f6 into main Oct 30, 2024
47 checks passed
@cscheid cscheid deleted the test/quarto-require-env-var branch October 30, 2024 18:39
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.

QUARTO_VERSION_REQUIREMENT env var

2 participants