feat: add `include_str` and `include_bytes` file input behaviour by lucascool12 · Pull Request #297 · la10736/rstest

lucascool12 · 2025-02-09T10:45:33Z

Adds the ability to set the file input format using the #[file_mode = "..."] attribute. The attribute accepts path, include_str, and include_bytes. path is the default behaviour, include_str and include_bytes pass the file contents instead of the path of the file.

Closes #295.

In the original issue I suggested using a #[embed] attribute but instead I decided to use the names include_str and include_bytes so that it is easier to find.

I'm not sure if file_mode is a good name for this.

la10736

Great work!

Just some notes:

I prefer a simple syntax like #[mode = (path|str|bytes)]: no just mode, no " that lead to think to something like free text, and smaller names
I would not ignore typo error and replace it with default mode, I prefer to give a clear error with the correct span

la10736 · 2025-02-10T09:05:09Z

rstest_macros/src/parse/rstest/files.rs

+#[derive(Debug, Clone, PartialEq, Eq)]
+pub(crate) enum FilesMode {
+    Path,
+    IncludeStr,
+    IncludeBytes,
+}
+
+impl Default for FilesMode {
+    fn default() -> Self {
+        Self::Path
+    }
+}


Suggested change

#[derive(Debug, Clone, PartialEq, Eq)]

pub(crate) enum FilesMode {

Path,

IncludeStr,

IncludeBytes,

}

impl Default for FilesMode {

fn default() -> Self {

Self::Path

}

}

#[derive(Debug, Clone, PartialEq, Eq)]

pub(crate) enum FilesMode {

#[default]

Path,

IncludeStr,

IncludeBytes,

}

la10736

Just to add some other notes...

Please don't forget to add a changelog line too.

la10736 · 2025-02-10T09:15:20Z

rstest_macros/src/parse/rstest/files.rs

+                LitStrAttr::parse_meta_name_value(&attr).map_err(|_| {
+                    attr.error(
+                        "Use #[file_mode = \"...\"] to define the argument of the file input",
+                    )
+                })


Here you can try to parse directly a FilesMode (should implement a Parse trait) that get an Ident and try to match it whit the expected varaints.

la10736 · 2025-02-10T09:16:59Z

rstest_macros/src/parse/rstest/files.rs

+                    .push(attr.error(r#"Cannot use #[file_mode = "..."] more than once"#))
+            });
+            FilesMode::from_str(&value.value())
+                .unwrap_or(Default::default())


That's is quite awful... if the user wrote a typo I would notice to him

rstest_macros/src/parse/rstest/files.rs

la10736 · 2025-02-10T09:25:11Z

rstest/tests/rstest/mod.rs

Thanks... I really appreciate your attention to test.

Can you please add also some test that check a correct error message when we use a wrong mode: you can add a case to the test module that check error messages for the other cases.

lucascool12 · 2025-02-12T19:58:28Z

Thank you for the feedback. I think I addressed all of your comments in a3e7fba.

la10736

Great, thanks again. Let me know what you think about my comment on println! on the build script.

la10736 · 2025-02-13T08:16:37Z

rstest/src/lib.rs

+/// Note on `println!("cargo::rerun-if-changed=tests/resources");`:
+/// All files that get embedded by [include_str] or [include_bytes] will be checked for changes
+/// when recompiling. So the `println` is not strictly needed when all necessary files are included
+/// with `#[mode = str]` or `#[mode = bytes]`.


I think that's useless. println!("cargo::rerun-if-changed=tests/resources"); is not used to force recompile when the file body changes, but when something change in the folder tests/resources and recompilation should occur to add or remove tests. println!("cargo::rerun-if-changed=tests/resources"); still need if you would like that cargo test catch if you add or remove some test cases by add or remove files in this folder.

Ah right you still need the println! for when files get added or removed even when using include_str or include_bytes. Then the note is not correct. I'll remove it.

Adds the ability to set the file input format using the `#[file_mode = "..."]` attribute. The attribute accepts `path`, `include_str`, and `include_bytes`. `path` is the default behaviour, `include_str` and `include_bytes` pass the file contents instead of the path of the file. Closes la10736#295.

la10736

LGTM

lucascool12 · 2025-02-19T12:08:34Z

Does the failing builds have anything to do with this PR?

martin-g · 2025-03-08T13:26:47Z

rstest/src/lib.rs

+/// fn for_each_path(
+///     #[files("src/**/*.rs")] #[exclude("test")] #[mode = path] path: PathBuf
+/// ) {
+///     assert!(contents.len() >= 0)


What is contents here ?
We have path, but no contents in scope.
It looks like a copy/paste error

it seems like I made a copy and paste error. But I don't understand how this doesn't make the doc tests fail.

Maybe because the doc test with rstest annotation just render the cases and mark the original function with #[cfg(test)]. The doc test doesn't compile with the test attribute, so if you did some mistakes in the function body they'll be ignored

Can you open a PR with the fix please?

Maybe because the doc test with rstest annotation just render the cases and mark the original function with #[cfg(test)]. The doc test doesn't compile with the test attribute, so if you did some mistakes in the function body they'll be ignored

This is unfortunate. I commented out these #[cfg(test)] with this patch and found that the doc example in this thread and this other doc example do not compile.

Maybe it is worth to spend some time trying to make it so these functions do get compiled? A quick (but also dirty) method would be to replace the #[cfg(test)] with a #[cfg(any(test, docttest))].

diff --git i/rstest_macros/src/render/mod.rs w/rstest_macros/src/render/mod.rs index 2a08918..80157e2 100644 --- i/rstest_macros/src/render/mod.rs +++ w/rstest_macros/src/render/mod.rs @@ -437,10 +437,10 @@ fn test_group(mut test: ItemFn, rendered_cases: TokenStream) -> TokenStream { test.attrs = vec![]; quote! { - #[cfg(test)] + // #[cfg(test)] #test - #[cfg(test)] + // #[cfg(test)] mod #fname { use super::*;

Can you open a PR with the fix please?

I opened #301.

Nice catch, I'll try to book some time in the launch block to do it. Thanks

Sadly I cannot find a way to do it. Also the quick and dirty solution still not works. It's a well know issue https://users.rust-lang.org/t/is-the-doctest-compiler-flag-broken-or-is-my-understanding/86477 rust-lang/rust#67295

la10736 requested changes Feb 10, 2025

View reviewed changes

lucascool12 force-pushed the feat-embedding-files branch from 6ee9dd1 to a3e7fba Compare February 12, 2025 19:55

lucascool12 force-pushed the feat-embedding-files branch from a3e7fba to 3c4701e Compare February 12, 2025 20:08

la10736 reviewed Feb 13, 2025

View reviewed changes

lucascool12 force-pushed the feat-embedding-files branch from 3c4701e to 4cafedb Compare February 13, 2025 08:29

la10736 approved these changes Feb 13, 2025

View reviewed changes

la10736 merged commit 8551eb8 into la10736:master Mar 2, 2025
2 checks passed

martin-g reviewed Mar 8, 2025

View reviewed changes

lucascool12 mentioned this pull request Mar 11, 2025

docs: fix filemode examples #301

Merged

Conversation

lucascool12 commented Feb 9, 2025

Uh oh!

la10736 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

la10736 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lucascool12 commented Feb 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

la10736 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

la10736 left a comment

Choose a reason for hiding this comment

Uh oh!

lucascool12 commented Feb 19, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

lucascool12 commented Feb 12, 2025 •

edited

Loading