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

Added feature --include-files #1667

Merged
merged 2 commits into from
Dec 30, 2024
Merged

Conversation

alt440
Copy link
Contributor

@alt440 alt440 commented Dec 26, 2024

Added the flag --include-files to tarpaulin to only have a select amount of files that we get coverage from. It can save some time, as we are not looking at the traces of excluded files.

A condition has been added in the include_path function to make sure that --include-files is deactivated when no argument has been passed: otherwise, this would exclude all files from the report (as --include-files excludes all files by default).

Pretty much copied from the --exclude-files feature. Added some tests to make sure everything was working fine.

Here are some commands that I tested on another Rust project from the built image:
cargo tarpaulin --include-files src/jwt/mod.rs --out xml --> SUCCESS only 5 lines seen, 60% coverage
cargo tarpaulin --include-files src/jwt/* --exclude-files src/utils/* --out xml --> SUCCESS only 5 lines seen, 60% coverage.
cargo tarpaulin --exclude-files src/utils/* --out xml --> SUCCESS 101 lines seen, 3% coverage

Copy link
Owner

@xd009642 xd009642 left a comment

Choose a reason for hiding this comment

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

A few comments mostly minor things, it would also be nice if the tarpaulin.toml in the root is updated to add the include-files field as this is the "mega config example" where the entire config is presented.

Oh also the fmt check is failing so if you do a cargo fmt and commit the results as well when all that's done!

@@ -976,6 +1017,30 @@ mod tests {
}
}

#[test]
fn include_paths_directory_separators() {
let args = TarpaulinCli::parse_from(vec![
Copy link
Owner

Choose a reason for hiding this comment

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

Because we discussed it in DMs just figured I'd put it here. You should also be able to create a config object with the include-files set like:

let config = Config {
	included_files_raw: vec!["src/foo/*".to_string(), "src\\bar\\*".to_string()],
        ..Default::default()
};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was really eager to try it!

But then, my excitement faded when I saw this:
imgTestOpenSource

included_files_raw is a private variable, just like excluded_files_raw, and for that reason I cannot set them this way.

As a bonus, I have added the LineAnalysis::new() error message. I will have issues with that as well.

I have been told that I should not make stuff public for the sole purpose of tests, but it's your call.

Do you want me to make those elements public?

Copy link
Owner

Choose a reason for hiding this comment

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

Oh right those are in a different module. That would only work in the tests in src/config/mod.rs

Outside the config module I'd just use the CLI Args to create the config type

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I decided to go against the test in the src/test_loader.rs file. It wouldn't be possible: even the creation of a LineAnalysis object is not possible for the test. Plus, I already tried TarpaulinCli.parse_from method and it is not accessible

src/path_utils.rs Outdated Show resolved Hide resolved
src/config/mod.rs Outdated Show resolved Hide resolved
src/test_loader.rs Outdated Show resolved Hide resolved
src/test_loader.rs Outdated Show resolved Hide resolved
@xd009642
Copy link
Owner

Cool LGTM now, I'll merge this and look at cutting a new release 🎉

@xd009642 xd009642 merged commit 0867478 into xd009642:develop Dec 30, 2024
20 of 21 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.

2 participants