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

Strip common path prefix #247

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

ivan23kor
Copy link

@ivan23kor ivan23kor commented Apr 25, 2024

Strip common path prefixes in test case names, generated with macro #files.

Implementation

After test file paths have been built relative to the base dir (CARGO_MANIFEST_DIR ), find the maximum common prefix for all test paths and remove this prefix from the test case names.

Example

rstest/tests/rstest/mod.rs demonstrates the gist of the change:

Common prefix _files is stripped from the paths:

-        .ok("start_with_name::path_1__UP_files_test_sub_folder_from_parent_folder_txt")
-        .ok("start_with_name::path_2_files_element_0_txt")
-        .ok("start_with_name::path_3_files_element_1_txt")
-        .ok("start_with_name::path_4_files_element_2_txt")
-        .ok("start_with_name::path_5_files_element_3_txt")
-        .ok("start_with_name::path_6_files_sub_sub_dir_file_txt")
-        .ok("start_with_name_with_include::path_1_files__ignore_me_txt")
-        .ok("start_with_name_with_include::path_2_files_element_0_txt")
-        .ok("start_with_name_with_include::path_3_files_element_1_txt")
-        .ok("start_with_name_with_include::path_4_files_element_2_txt")
-        .ok("start_with_name_with_include::path_5_files_element_3_txt")
-        .ok("start_with_name_with_include::path_6_files_sub_sub_dir_file_txt")
+        .ok("start_with_name::path_1_files_test_sub_folder_from_parent_folder_txt")
+        .ok("start_with_name::path_2_rstest_files_files_element_0_txt")
+        .ok("start_with_name::path_3_rstest_files_files_element_1_txt")
+        .ok("start_with_name::path_4_rstest_files_files_element_2_txt")
+        .ok("start_with_name::path_5_rstest_files_files_element_3_txt")
+        .ok("start_with_name::path_6_rstest_files_files_sub_sub_dir_file_txt")
+        .ok("start_with_name_with_include::path_1__ignore_me_txt")
+        .ok("start_with_name_with_include::path_2_element_0_txt")
+        .ok("start_with_name_with_include::path_3_element_1_txt")
+        .ok("start_with_name_with_include::path_4_element_2_txt")
+        .ok("start_with_name_with_include::path_5_element_3_txt")
+        .ok("start_with_name_with_include::path_6_sub_sub_dir_file_txt")

Details

  1. Test cases for #files have been updated to expect common prefix stripped.
  2. Minor refactoring: use a simple literal expression for path values instead of conversion expression between String and PathBuf in tests for #files.

Addresses #212.

Summary:
1. After gathering paths test files matching the glob pattern and
building paths to these test files, relative to a base directory
(CARGO_MANIFEST_DIR by default), find the maximum common prefix for all
the test cases and strip that prefix from each test case name.
2. Update test cases for #files to strip common prefix from expected
paths.
@ivan23kor
Copy link
Author

@la10736 I just noticed through local testing that 7e1f611 introduced unintended behaviour - it does not respect exclude glob. 278ccdc still works. I am currently debugging.

attr.error(&format!("Invalid absolute path {}", e.to_string_lossy()))
})?;

for (abs_path, relative_path) in abs_paths.iter().zip(relative_paths) {
if !refs.is_valid(&relative_path) {
Copy link
Author

Choose a reason for hiding this comment

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

@la10736 this is_valid applies the original glob pattern path to the truncated path, hence path exclusion will not work after prefix stripping. I will move is_valid check to before path stripping.

Copy link
Author

@ivan23kor ivan23kor Apr 30, 2024

Choose a reason for hiding this comment

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

Fixed it in 76671c7: I changed is_valid -> !is_excluded and moved is_excluded into find_paths, so find_paths now applies both include and exclude globs.

@la10736
Copy link
Owner

la10736 commented Apr 29, 2024

THX for this PR!! Really appreciated!
Sorry, I need some time for review it... but I'll do it ASAP...

@ivan23kor
Copy link
Author

No rush and thank you for getting to reviewing it.

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.

None yet

2 participants