Skip to content

Double-check cfg_version test coverage #141452

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

Closed
jieyouxu opened this issue May 23, 2025 · 2 comments · Fixed by #141552
Closed

Double-check cfg_version test coverage #141452

jieyouxu opened this issue May 23, 2025 · 2 comments · Fixed by #141552
Assignees
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc C-cleanup Category: PRs that clean code up or issues documenting cleanup. F-cfg_version `#![feature(cfg_version)]` T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@jieyouxu
Copy link
Member

          Discussion: this test is kinda testing multiple things and bears multiple responsibilities, we may want to split this test up:
  1. We wish to exercise the feature-gating of the unstable feature itself. That is, I would expect #[cfg(version("1.49.0"))] to be this feature-gate test without #![feature(cfg_version)] and the user should get the "this is unstable" error message.
  2. We wish to exercise the wellformed-ness of the values passed to version(..) as a sort of "interface". In particular, I'd consider splitting tests for:
    1. The syntactical wellformedness:
      • Proper name-with-paren-value form (version(..)): known vs unknown major version string "1", known vs unknown major-minor version string, known vs unknown major-minor-patch version string, empty version string, some "hello world" nonsense version string.
      • Invalid forms (version = ".." or bare version).
    2. The semantic wellformedness of the version string: major version (e.g. "1"), major-minor version (e.g. "1.1"), major-minor-patch (e.g. "1.1.1"), and also known-vs-unknown variants for them.
  3. I'd also check that if user manually specifies --cfg version or --cfg version=".." they are not affected and usable with or without feature gate (I need to double-check that).

This is also fine as a follow-up, I can probably write those coverage.

Originally posted by @jieyouxu in #141413 (comment)

@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label May 23, 2025
@jieyouxu jieyouxu self-assigned this May 23, 2025
@jieyouxu jieyouxu added C-cleanup Category: PRs that clean code up or issues documenting cleanup. A-testsuite Area: The testsuite used to check the correctness of rustc T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. F-cfg_version `#![feature(cfg_version)]` and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels May 23, 2025
@est31
Copy link
Member

est31 commented May 24, 2025

As written in #141413 (comment), test feature-gate-cfg-version.rs covers most of the points (1, 2).

@jieyouxu
Copy link
Member Author

(3) is probably too pendatic.

jieyouxu added a commit to jieyouxu/rust that referenced this issue May 26, 2025
Pull out dedicated `cfg_version` syntax test from feature gate test

Tracking issue: rust-lang#64796.
Closes rust-lang#141452, as a follow-up to rust-lang#141413 (comment) (point 3 of that is probably too pedantic).

The feature gate test was dual-purposing causing feature gate errors to distract from syntax exercises.

`@rustbot` label +F-cfg_version
r? `@est31`
@bors bors closed this as completed in b7854c6 May 27, 2025
rust-timer added a commit that referenced this issue May 27, 2025
Rollup merge of #141552 - jieyouxu:cfg-version-tests, r=est31

Pull out dedicated `cfg_version` syntax test from feature gate test

Tracking issue: #64796.
Closes #141452, as a follow-up to #141413 (comment) (point 3 of that is probably too pedantic).

The feature gate test was dual-purposing causing feature gate errors to distract from syntax exercises.

``@rustbot`` label +F-cfg_version
r? ``@est31``
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc C-cleanup Category: PRs that clean code up or issues documenting cleanup. F-cfg_version `#![feature(cfg_version)]` T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants