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

merged doctests: regressions due to examining process arguments #130796

Closed
ehuss opened this issue Sep 24, 2024 · 1 comment · Fixed by #131095
Closed

merged doctests: regressions due to examining process arguments #130796

ehuss opened this issue Sep 24, 2024 · 1 comment · Fixed by #131095
Labels
A-doctests Area: Documentation tests, run by rustdoc A-edition-2024 Area: The 2024 edition C-bug Category: This is a bug. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.

Comments

@ehuss
Copy link
Contributor

ehuss commented Sep 24, 2024

The crater run for merged doctests (#130285) detected a large number of regressions due to tests examining the process arguments. (I don't have an exact number, but let's say ~50 projects.)

Previously, there were no arguments, but now it gets arguments like *doctest-inner-test 0 which the user's CLI parsing code isn't expecting.

For example, using something like StructOpt::from_args will now fail.

It seems to be fairly common to have objects whose default constructor will parse the arguments from the command line.

I'm not sure if or how we should resolve that. One idea is to use a different mechanism for passing in the test information (like via an environment variable).

Meta

rustc 1.83.0-nightly (da889684c 2024-09-20)
binary: rustc
commit-hash: da889684c80508036ff036db8c159ffdcf27648a
commit-date: 2024-09-20
host: aarch64-apple-darwin
release: 1.83.0-nightly
LLVM version: 19.1.0
@ehuss ehuss added A-doctests Area: Documentation tests, run by rustdoc A-edition-2024 Area: The 2024 edition C-bug Category: This is a bug. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Sep 24, 2024
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Sep 24, 2024
@saethlin saethlin removed the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Sep 24, 2024
@GuillaumeGomez
Copy link
Member

I vaguely remember that using environment variables was discussed but can't find anything except mentioning it here. Maybe it'd be a better approach indeed. Let me send a PR so we can make a crater run on it.

bors added a commit to rust-lang-ci/rust that referenced this issue Oct 1, 2024
…es, r=<try>

Use environment variables instead of command line arguments for merged doctests

Fixes [rust-lang#130796](rust-lang#130796).

Before merging this one, let's first ensure it has a smaller impact compared to command line arguments (results can be seen [here](rust-lang#130285)). I'll start a crater run once CI passed.

cc `@ehuss`
r? `@notriddle`
bors added a commit to rust-lang-ci/rust that referenced this issue Oct 1, 2024
…es, r=<try>

Use environment variables instead of command line arguments for merged doctests

Fixes [rust-lang#130796](rust-lang#130796).

Before merging this one, let's first ensure it has a smaller impact compared to command line arguments (results can be seen [here](rust-lang#130285)). I'll start a crater run once CI passed.

cc `@ehuss`
r? `@notriddle`
compiler-errors added a commit to compiler-errors/rust that referenced this issue Oct 15, 2024
…bles, r=notriddle

Use environment variables instead of command line arguments for merged doctests

Fixes [rust-lang#130796](rust-lang#130796).

Before merging this one, let's first ensure it has a smaller impact compared to command line arguments (results can be seen [here](rust-lang#130285)). I'll start a crater run once CI passed.

cc `@ehuss`
r? `@notriddle`
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Oct 15, 2024
…bles, r=notriddle

Use environment variables instead of command line arguments for merged doctests

Fixes [rust-lang#130796](rust-lang#130796).

Before merging this one, let's first ensure it has a smaller impact compared to command line arguments (results can be seen [here](rust-lang#130285)). I'll start a crater run once CI passed.

cc ``@ehuss``
r? ``@notriddle``
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Oct 15, 2024
Rollup merge of rust-lang#131095 - GuillaumeGomez:switch-to-env-variables, r=notriddle

Use environment variables instead of command line arguments for merged doctests

Fixes [rust-lang#130796](rust-lang#130796).

Before merging this one, let's first ensure it has a smaller impact compared to command line arguments (results can be seen [here](rust-lang#130285)). I'll start a crater run once CI passed.

cc ``@ehuss``
r? ``@notriddle``
github-actions bot pushed a commit to rust-lang/miri that referenced this issue Oct 17, 2024
…otriddle

Use environment variables instead of command line arguments for merged doctests

Fixes [#130796](rust-lang/rust#130796).

Before merging this one, let's first ensure it has a smaller impact compared to command line arguments (results can be seen [here](rust-lang/rust#130285)). I'll start a crater run once CI passed.

cc ``@ehuss``
r? ``@notriddle``
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-doctests Area: Documentation tests, run by rustdoc A-edition-2024 Area: The 2024 edition C-bug Category: This is a bug. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
Development

Successfully merging a pull request may close this issue.

4 participants