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

refactor(test): rust integration tests #9314

Merged
merged 17 commits into from
Oct 25, 2024

Conversation

NicholasLYang
Copy link
Contributor

@NicholasLYang NicholasLYang commented Oct 22, 2024

Description

Fleshing out the rust integration tests. Added a macro for creating an integration test. The macro isn't perfect since it doesn't support running non-turbo commands for setup, and multiple arguments aren't implemented.

Testing Instructions

@NicholasLYang NicholasLYang requested a review from a team as a code owner October 22, 2024 20:47
Copy link

vercel bot commented Oct 22, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
examples-nonmonorepo ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 25, 2024 1:48pm
8 Skipped Deployments
Name Status Preview Comments Updated (UTC)
examples-basic-web ⬜️ Ignored (Inspect) Visit Preview Oct 25, 2024 1:48pm
examples-designsystem-docs ⬜️ Ignored (Inspect) Visit Preview Oct 25, 2024 1:48pm
examples-gatsby-web ⬜️ Ignored (Inspect) Visit Preview Oct 25, 2024 1:48pm
examples-kitchensink-blog ⬜️ Ignored (Inspect) Visit Preview Oct 25, 2024 1:48pm
examples-native-web ⬜️ Ignored (Inspect) Visit Preview Oct 25, 2024 1:48pm
examples-svelte-web ⬜️ Ignored (Inspect) Visit Preview Oct 25, 2024 1:48pm
examples-tailwind-web ⬜️ Ignored (Inspect) Visit Preview Oct 25, 2024 1:48pm
examples-vite-web ⬜️ Ignored (Inspect) Visit Preview Oct 25, 2024 1:48pm

Copy link
Member

@chris-olszewski chris-olszewski left a comment

Choose a reason for hiding this comment

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

Some small things, but super excited for the follow up PRs that remove the corresponding Prysk tests

crates/turborepo/tests/common/mod.rs Outdated Show resolved Hide resolved
.output()?;

let stdout = String::from_utf8(output.stdout)?;
let query_output: serde_json::Value = serde_json::from_str(&stdout)?;
Copy link
Member

Choose a reason for hiding this comment

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

Might be worth changing the macro name/doc comments to make it clear this is useful only for commands that produce JSON output

Copy link
Member

Choose a reason for hiding this comment

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

Minor, but there still isn't mention in the doc comment or macro name that this is for JSON command only.

crates/turborepo/tests/common/mod.rs Show resolved Hide resolved
$name.replace(' ', "_"),
$package_manager
);
insta::assert_json_snapshot!(test_name, query_output);
Copy link
Member

Choose a reason for hiding this comment

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

Can we put each of these asserts in it's own test? Currently if the first test case fails, none of the other ones will run. Prysk will continue running if one of the commands doesn't produce matching output.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a way to do this while also using the same fixture? I don't want to spin up a fixture for each test. Insta does also let you do the prysk behavior with cargo insta test --review or INSTA_FORCE_PASS=1 cargo test --no-fail-fast

Copy link
Member

Choose a reason for hiding this comment

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

I suppose we'd have to store the tempfile in something like a static ref and have special cleanup logic. I think eventually we'll want beforeAll/afterAll behavior to make debugging failures easier. ITG

Copy link
Member

Choose a reason for hiding this comment

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

Insta does also let you do the prysk behavior with cargo insta test --review or INSTA_FORCE_PASS=1 cargo test --no-fail-fast

I don't think these do what we want, INSTA_FORCE_PASS=1 seems to just forcibly pass all snapshot assertions without printing any diffs. cargo insta test --review is just equivalent to --interactive in prysk.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

According to this those are the recommended ways to not fail on the first test

Co-authored-by: Chris Olszewski <chris.olszewski@vercel.com>

pub fn setup_fixture(
fixture: &str,
package_manager: Option<&str>,
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: it would be nice to keep this explicit so it's really clear from looking at the test what it's gonna run with. or, if not, it'd be nice if there was some way to make check! variadic (is there?).

Copy link
Member

@chris-olszewski chris-olszewski left a comment

Choose a reason for hiding this comment

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

ITG

$name.replace(' ', "_"),
$package_manager
);
insta::assert_json_snapshot!(test_name, query_output);
Copy link
Member

Choose a reason for hiding this comment

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

I suppose we'd have to store the tempfile in something like a static ref and have special cleanup logic. I think eventually we'll want beforeAll/afterAll behavior to make debugging failures easier. ITG

.output()?;

let stdout = String::from_utf8(output.stdout)?;
let query_output: serde_json::Value = serde_json::from_str(&stdout)?;
Copy link
Member

Choose a reason for hiding this comment

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

Minor, but there still isn't mention in the doc comment or macro name that this is for JSON command only.

$name.replace(' ', "_"),
$package_manager
);
insta::assert_json_snapshot!(test_name, query_output);
Copy link
Member

Choose a reason for hiding this comment

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

Insta does also let you do the prysk behavior with cargo insta test --review or INSTA_FORCE_PASS=1 cargo test --no-fail-fast

I don't think these do what we want, INSTA_FORCE_PASS=1 seems to just forcibly pass all snapshot assertions without printing any diffs. cargo insta test --review is just equivalent to --interactive in prysk.

expression: stderr
---
WARNING No locally installed `turbo` found. Using version: 2.2.4-canary.0.
turbo 2.2.4-canary.0
Copy link
Member

Choose a reason for hiding this comment

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

We don't want to capture this since we always use newest turbo version

@NicholasLYang NicholasLYang merged commit ddf7316 into main Oct 25, 2024
40 checks passed
@NicholasLYang NicholasLYang deleted the refactor/add-more-rust-integration-tests branch October 25, 2024 14:53
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.

3 participants