Skip to content

feat: support outer doc comments for naming matrix tests#321

Merged
la10736 merged 1 commit intola10736:masterfrom
orhun:feat/support_matrix_test_name
Nov 26, 2025
Merged

feat: support outer doc comments for naming matrix tests#321
la10736 merged 1 commit intola10736:masterfrom
orhun:feat/support_matrix_test_name

Conversation

@orhun
Copy link
Contributor

@orhun orhun commented Jul 10, 2025

Adds support for using outer doc comments (///) instead of
auto-generated tests names for parameterized matrix tests.

e.g. legacy compact syntax:

expected => [
        /// forty_two
        42,
        /// four
        2*3-2,
    ],
)]
fn test(expected: usize) { ... }

e.g. attribute syntax:

fn test(
    #[values(
        /// forty_two
        42,
        /// four
        2*3-2,
    )]
    expected: usize,
) { ... }

The generated test names will be values_1_len_forty_two and
values_2_len_four.

Fixes #320

@la10736
Copy link
Owner

la10736 commented Jul 11, 2025

Ok, it looks fine as POC. Just a note about description, I prefer something like All_present and No_data_file. I don't remember where the sanitization occurs but I hope we can find a good way to do it.

Of course, both unit and integration tests are missed, but it's a POC.

Last note: I'm on vacation till Jul 21th, so I can't review this PR till this date.

Thx

@orhun orhun force-pushed the feat/support_matrix_test_name branch 2 times, most recently from 38aa283 to bf4da40 Compare July 11, 2025 08:02
@orhun
Copy link
Contributor Author

orhun commented Jul 11, 2025

I prefer something like All_present and No_data_file

Good point, updated the PR!

I added an additional parameter to sanitize_ident for disabling whitepspace filtering (to not break the previous behavior) and now are turned into _.

Let me know if this solution is good enough!

both unit and integration tests are missed

I added an unit test in the vlist module. For the integration tests, where would be a good place? Maybe under rtest/tests somewhere? I'd appreciate some guidance there :D

I'm on vacation till Jul 21th, so I can't review this PR till this date.

All good, no rush :)

Copy link
Owner

@la10736 la10736 left a comment

Choose a reason for hiding this comment

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

First of all thanks very much for your effort and please apologize me for my late review.

You had implement the parser just for the older and deprecated syntax https://docs.rs/rstest/latest/rstest/attr.rstest.html#old-compact-syntax but not for the newer one https://docs.rs/rstest/latest/rstest/attr.rstest.html#values-lists . Moreover you introduced a silly bug (maybe I should write an integration test for it).

Here a todo list

  • Fix legacy parser
  • Implement the parser for the newer syntax too (with some test unit)
  • Add integration tests: expand matrix::use_args_attributes in rstest/tests/rstest/mod.rs for the newer syntax and matrix::happy_path for the older.
  • Modify `#[values] documentation (just for the newer syntax is enough)
  • Add a line in changelog file

@orhun orhun force-pushed the feat/support_matrix_test_name branch 5 times, most recently from 8b542af to 1220107 Compare November 17, 2025 17:57
@orhun orhun requested a review from la10736 November 17, 2025 17:58
@orhun
Copy link
Contributor Author

orhun commented Nov 17, 2025

I addressed the feedback, please have a look again!

@orhun orhun force-pushed the feat/support_matrix_test_name branch from 1220107 to dbe095f Compare November 17, 2025 18:01
Copy link
Owner

@la10736 la10736 left a comment

Choose a reason for hiding this comment

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

It's a great job but two more things:

  1. I would have test for both case: with and without doc comments
  2. I guess that the changes to sanitize_ident are not necessary (add a boolean flag to a function is often far than the best). Can you try my proposed solution?

@orhun orhun force-pushed the feat/support_matrix_test_name branch 3 times, most recently from 8df3567 to 5368d61 Compare November 22, 2025 19:03
@orhun
Copy link
Contributor Author

orhun commented Nov 22, 2025

Thanks for the review. Made the changes, please have a look!

@orhun orhun requested a review from la10736 November 22, 2025 19:04
Copy link
Owner

@la10736 la10736 left a comment

Choose a reason for hiding this comment

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

Great Job!! Thanks a Lot. Just last small thing: change the changelog not from change to add

Adds support for using outer doc comments (`///`) instead of
auto-generated tests names for parameterized matrix tests.

e.g. legacy compact syntax:

```rust
expected => [
        /// forty_two
        42,
        /// four
        2*3-2,
    ],
)]
fn test(expected: usize) { ... }
```

e.g. attribute syntax:

```rust
fn test(
    #[values(
        /// forty_two
        42,
        /// four
        2*3-2,
    )]
    expected: usize,
) { ... }
```

The generated test names will be `values_1_len_forty_two` and
`values_2_len_four`.

Fixes <la10736#320>

Signed-off-by: Orhun Parmaksız <orhunparmaksiz@gmail.com>
@orhun orhun force-pushed the feat/support_matrix_test_name branch from 5368d61 to e588c39 Compare November 25, 2025 16:37
@orhun
Copy link
Contributor Author

orhun commented Nov 25, 2025

all done!

@la10736 la10736 merged commit 454df84 into la10736:master Nov 26, 2025
3 checks passed
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.

Set test names for matrix tests

2 participants