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

Jump to bottom

Run doctests as part of CI pipeline #9939

Merged
merged 1 commit into from
Feb 12, 2024
Merged

Run doctests as part of CI pipeline #9939

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
8 changes: 8 additions & 0 deletions .config/nextest.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
[profile.ci]
Copy link
Member Author

Choose a reason for hiding this comment

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

cargo insta doesn't support passing custom test runner flags. Luckily, nextest supports setting the profile via environment flag..

# Print out output for failing tests as soon as they fail, and also at the end
# of the run (for easy scrollability).
failure-output = "immediate-final"
# Do not cancel the test run on the first failure.
fail-fast = false

status-level = "skip"
13 changes: 11 additions & 2 deletions .github/workflows/ci.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,10 +117,17 @@ jobs:
uses: taiki-e/install-action@v2
with:
tool: cargo-nextest
- name: "Install cargo insta"
uses: taiki-e/install-action@v2
with:
tool: cargo-insta
- uses: Swatinem/rust-cache@v2
- name: "Run tests"
shell: bash
run: cargo nextest run --workspace --status-level skip --failure-output immediate-final --no-fail-fast -j 12
Copy link
Member Author

Choose a reason for hiding this comment

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

I removed the -j 12. It gives us minimal speedup but has the downside that it requires manual updating (that likely doesn't happen) when any properties about the runners change.

Copy link
Member

Choose a reason for hiding this comment

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

Did you benchmark it? It seemed like it was about 30s faster for WIndows (#9921 (comment)).

Copy link
Member Author

@MichaReiser MichaReiser Feb 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I did not but you mentioned on your PR that it is minimal 😆

Basically no difference:

Copy link
Member

Choose a reason for hiding this comment

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

Lol you got me

Copy link
Member

Choose a reason for hiding this comment

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

I ended up keeping it because the Windows difference was kinda big though...

env:
NEXTEST_PROFILE: "ci"
run: cargo insta test --all-features --unreferenced reject --test-runner nextest
Copy link
Member

Choose a reason for hiding this comment

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

Oh I didn't know about this, huh. Did you know about this @zanieb?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I talked about this in my earlier pull request I linked :p I think the key here is setting the profile via environment variable? Nice work @MichaReiser


# Check for broken links in the documentation.
- run: cargo doc --all --no-deps
env:
Expand Down Expand Up @@ -148,7 +155,9 @@ jobs:
- uses: Swatinem/rust-cache@v2
- name: "Run tests"
shell: bash
run: cargo nextest run --workspace --status-level skip --failure-output immediate-final --no-fail-fast -j 12
run: |
cargo nextest run --all-features --profile ci
cargo test --all-features --doc
Copy link
Member

Choose a reason for hiding this comment

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

Why is this on the Windows tests? If anything, shouldn't it only be on the Linux tests, since doctests add so much overhead, and Windows is the bottleneck?

Copy link
Member Author

@MichaReiser MichaReiser Feb 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

cargo insta uses nextest to run the "normal" test but calls cargo test --doc for doctests.

So the doctests run on both Windows and Linux.

However, we never used the unreferenced-snapshots feature on Windows, so it felt useless to use cargo insta on Windows for a feature that we don't need. That's why it makes the call directly.

Copy link
Member

Choose a reason for hiding this comment

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

Ah that makes sense, thanks! Doctests are so slow, honestly I'm tempted not to run them on Windows.

cargo-test-wasm:
name: "cargo test (wasm)"
Expand Down
1 change: 1 addition & 0 deletions crates/ruff_benchmark/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ license = { workspace = true }

[lib]
bench = false
doctest = false

[[bench]]
name = "linter"
Expand Down
1 change: 1 addition & 0 deletions crates/ruff_diagnostics/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ repository = { workspace = true }
license = { workspace = true }

[lib]
doctest = false

[dependencies]
ruff_text_size = { path = "../ruff_text_size" }
Expand Down
1 change: 1 addition & 0 deletions crates/ruff_index/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ repository = { workspace = true }
license = { workspace = true }

[lib]
doctest = false

[dependencies]
ruff_macros = { path = "../ruff_macros" }
Expand Down
4 changes: 3 additions & 1 deletion crates/ruff_macros/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,9 @@ pub fn derive_message_formats(_attr: TokenStream, item: TokenStream) -> TokenStr
///
/// Good:
///
/// ```rust
/// ```ignroe
/// use ruff_macros::newtype_index;
///
/// #[newtype_index]
/// #[derive(Ord, PartialOrd)]
/// struct MyIndex;
Expand Down
1 change: 1 addition & 0 deletions crates/ruff_notebook/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ repository = { workspace = true }
license = { workspace = true }

[lib]
doctest = false

[dependencies]
ruff_diagnostics = { path = "../ruff_diagnostics" }
Expand Down
1 change: 1 addition & 0 deletions crates/ruff_python_codegen/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ repository = { workspace = true }
license = { workspace = true }

[lib]
doctest = false

[dependencies]
ruff_python_ast = { path = "../ruff_python_ast" }
Expand Down
3 changes: 3 additions & 0 deletions crates/ruff_python_formatter/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,9 @@ documentation = { workspace = true }
repository = { workspace = true }
license = { workspace = true }

[lib]
doctest= false

[dependencies]
ruff_cache = { path = "../ruff_cache" }
ruff_formatter = { path = "../ruff_formatter" }
Expand Down
1 change: 1 addition & 0 deletions crates/ruff_python_index/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ repository = { workspace = true }
license = { workspace = true }

[lib]
doctest = false

[dependencies]
ruff_python_ast = { path = "../ruff_python_ast" }
Expand Down
3 changes: 3 additions & 0 deletions crates/ruff_python_literal/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,9 @@ documentation = { workspace = true }
repository = { workspace = true }
license = { workspace = true }

[lib]
doctest = false

[dependencies]
bitflags = { workspace = true }
hexf-parse = { workspace = true }
Expand Down
1 change: 1 addition & 0 deletions crates/ruff_python_resolver/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ repository = { workspace = true }
license = { workspace = true }

[lib]
doctest = false

[dependencies]
log = { workspace = true }
Expand Down
1 change: 1 addition & 0 deletions crates/ruff_python_semantic/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ repository = { workspace = true }
license = { workspace = true }

[lib]
doctest = false

[dependencies]
ruff_index = { path = "../ruff_index" }
Expand Down
1 change: 1 addition & 0 deletions crates/ruff_wasm/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ description = "WebAssembly bindings for Ruff"

[lib]
crate-type = ["cdylib", "rlib"]
doctest = false

[features]
default = ["console_error_panic_hook"]
Expand Down
Loading