Skip to content

ctest: Add skips and mappings. #4514

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
merged 1 commit into from
Jul 7, 2025
Merged

Conversation

mbyx
Copy link
Contributor

@mbyx mbyx commented Jul 2, 2025

Description

This PR adds more methods to the frontend of TestGenerator, including the ability to skip items and map their identifiers and types to another if needed. This will be useful when porting ctest-test and for writing cleaner test templates for the other items in the future.

Sources

Checklist

  • Relevant tests in libc-test/semver have been updated
  • No placeholder or unstable values like *LAST or *MAX are
    included (see #3131)
  • Tested locally (cd libc-test && cargo test --target mytarget);
    especially relevant for platforms that may not be checked in CI

@rustbot rustbot added ctest Issues relating to the ctest crate S-waiting-on-review labels Jul 2, 2025
@tgross35
Copy link
Contributor

tgross35 commented Jul 2, 2025

I've been thinking a bit - I think that for the next PR, you should get tests running against the real libc, in parallel with the existing ctest. Since you have the skips, it should basically be possible to opt in to a subset with something like:

cfg.skip_static(c| 
    ![
        "some_static",
        "other_static",
    ].contains(c.name())
)

The reason I am suggesting this is because there is probably a lot that won't be obvious until this starts processing real code and we see the kind of failures that arise.

@mbyx mbyx force-pushed the ctest-frontend branch from 35398e0 to c7b8729 Compare July 3, 2025 09:10
@mbyx mbyx requested a review from tgross35 July 3, 2025 10:59
@mbyx mbyx force-pushed the ctest-frontend branch 2 times, most recently from 589366b to 6834c82 Compare July 3, 2025 11:24
Copy link
Contributor

@tgross35 tgross35 left a comment

Choose a reason for hiding this comment

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

Few small things then I think this should be good to go

@mbyx mbyx force-pushed the ctest-frontend branch from 6834c82 to 514e5c9 Compare July 7, 2025 10:42
@mbyx mbyx requested a review from tgross35 July 7, 2025 10:43
@mbyx mbyx force-pushed the ctest-frontend branch from 514e5c9 to 96333a2 Compare July 7, 2025 10:59
@mbyx mbyx requested a review from tgross35 July 7, 2025 10:59
@mbyx mbyx force-pushed the ctest-frontend branch from 96333a2 to 33b8583 Compare July 7, 2025 11:08
Copy link
Contributor

@tgross35 tgross35 left a comment

Choose a reason for hiding this comment

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

Few misc comments, mostly just doc grammar fixes. Looks like there is a test failure


/// Is field volatile?
///
/// This is used to make sure that the generated test code also has the volatile keyword.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// This is used to make sure that the generated test code also has the volatile keyword.
/// Indicate that a struct field should be marked `volatile`.

Complete sentence for the summary, then I don't think we need the rest

Comment on lines 181 to 183
/// Is static variable volatile?
///
/// This is used to make sure that the generated test code also has the volatile keyword.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// Is static variable volatile?
///
/// This is used to make sure that the generated test code also has the volatile keyword.
/// Indicate that a static should be marked `volatile`.

Comment on lines 206 to 208
/// Is function argument volatile?
///
/// This is used to make sure that the generated test code also has the volatile keyword.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// Is function argument volatile?
///
/// This is used to make sure that the generated test code also has the volatile keyword.
/// Indicate that a function argument should be marked `volatile`.

Comment on lines 234 to 236
/// Is function return type volatile?
///
/// This is used to make sure that the generated test code also has the volatile keyword.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// Is function return type volatile?
///
/// This is used to make sure that the generated test code also has the volatile keyword.
/// Indicate that a function's return type should be marked `volatile`.

self
}

/// Is argument of function an array?
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// Is argument of function an array?
/// Indicate that a function pointer argument is an array.

Comment on lines 55 to 66
Struct(&'a Struct),
Fn(&'a crate::Fn),
#[expect(unused)]
Field(&'a Struct, &'a Field),
Alias(&'a Type),
Const(&'a Const),
Static(&'a Static),
Type(&'a str),
StructType(&'a str),
UnionType(&'a str),
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add a doc to Struct and StructType showing the difference there?

@tgross35
Copy link
Contributor

tgross35 commented Jul 7, 2025

You'll probably need to rebase for the test failure. After that and the above fixes, please clean up history

@mbyx mbyx closed this Jul 7, 2025
@mbyx mbyx force-pushed the ctest-frontend branch from 33b8583 to 9ba4f41 Compare July 7, 2025 11:34
@mbyx mbyx reopened this Jul 7, 2025
@mbyx mbyx force-pushed the ctest-frontend branch 2 times, most recently from b679561 to 67fc632 Compare July 7, 2025 11:39
@mbyx mbyx force-pushed the ctest-frontend branch 4 times, most recently from ffc49ec to 27a2ae5 Compare July 7, 2025 11:56
@tgross35
Copy link
Contributor

tgross35 commented Jul 7, 2025

You just need to re-bless the tests no? The input files had to change at #4525, the new files added here don't reflect that.

@mbyx mbyx force-pushed the ctest-frontend branch from 27a2ae5 to 37999ea Compare July 7, 2025 11:58
@mbyx mbyx force-pushed the ctest-frontend branch from 37999ea to 77a4657 Compare July 7, 2025 12:00
Copy link
Contributor

@tgross35 tgross35 left a comment

Choose a reason for hiding this comment

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

Looks like it should pass now, on to the next one!

@tgross35 tgross35 enabled auto-merge July 7, 2025 12:02
@tgross35 tgross35 added this pull request to the merge queue Jul 7, 2025
Merged via the queue into rust-lang:main with commit 3e00495 Jul 7, 2025
49 of 52 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-CI Area: CI-related items ctest Issues relating to the ctest crate O-gnu O-linux O-mips O-musl O-powerpc O-unix O-x86
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants