[TRUNK-14658] Add relative test file support

[cmillar-trunk]

Adds two pieces of logic:

• Falls back to checking the test case classname for a path as well
• Convert relative paths to absolute paths, browsing through non-hidden folders in the repository. These two cover the ways vitest differs from every other test suite we've encountered.

[trunk-io]

Merging to main in this repository is managed by Trunk.

• To merge this pull request, check the box to the left or comment /trunk merge below.

[chip-agent]

Okay, I have retrieved the file changes and diffs for this pull request. It seems like the main changes are related to how file paths are handled within the junit test results, specifically for vitest.

Here's a summary of the changes:

• .github/actions/run_tests/action.yaml: Added --nocapture to the cargo nextest run command. This likely aims to display test output in the CI logs, which could be helpful for debugging.
• Cargo.lock and context/Cargo.toml: Added walkdir as a dependency. This crate is used for recursively walking directories, which is relevant to the file path changes.
• cli-tests/src/upload.rs: A new test case uploaded_file_contains_updated_test_files was added. This test seems to verify that the correct file paths are being uploaded in the bundle metadata.
• cli-tests/src/utils.rs: Modified the generate_mock_valid_junit_xmls function to accept a generic type and pass it to the generate_reports function.
• cli-tests/src/validate.rs: Modified the validate_suboptimal_junits test to use TempDir::with_prefix("not-hidden").unwrap() instead of tempdir().unwrap(). Also, generate_mock_git_repo was added to validate_missing_filepath_suboptimal_junits.
• cli/src/context.rs and cli/src/upload_command.rs: Changes to pass the BundleRepo to the generate_internal_file function.
• cli/src/validate_command.rs: Changes to create a BundleRepo and pass it to the validate_report function.
• context-js/src/lib.rs and context-py/src/lib.rs: Changes to pass a default BundleRepo to the validate function.
• context/src/junit/bindings.rs: Added a test case and modified the validate function to accept a BundleRepo.
• context/src/junit/file_extractor.rs: A new file file_extractor.rs was added. This file contains logic for extracting the filename for a test case from the JUnit XML, including handling relative paths and searching for the file in the repository.
• context/src/junit/mod.rs: Added mod file_extractor;.
• context/src/junit/parser.rs: Modified the into_test_case_runs function to accept a BundleRepo and use it to determine the file path of the test case.
• context/src/junit/validator.rs: Modified the validate function to accept a BundleRepo and use it to validate the file path of the test case.
• context/tests/junit.rs: Modified the test cases to use a BundleRepo.
• junit-mock/src/lib.rs: Modified the generate_reports function to accept a generic type and pass it to the generate_test_suites function. Also, modified the generate_test_cases function to create the file if it doesn't exist.

Based on these changes, it seems like the pull request is primarily focused on improving the handling of file paths in JUnit XML reports, especially for test frameworks like vitest that may use relative paths or unconventional naming schemes. The addition of walkdir and the new file_extractor.rs file strongly suggest that the code is now attempting to resolve relative file paths by searching the file system.

Given that the tests are reported as flaky, and the changes touch file path handling, it's possible that the flakiness is related to how the tests interact with the file system or how they resolve file paths. It's also possible that the --nocapture change in the CI configuration is affecting the test results in some way.

To investigate further, I would suggest the following:

1. Examine the CI logs more closely. Look for any error messages or warnings related to file path resolution or file system access. The --nocapture flag should make it easier to see the test output.
2. Check the new test case uploaded_file_contains_updated_test_files in cli-tests/src/upload.rs. See if this test is failing intermittently, as it directly tests the file path handling logic.
3. Consider the possibility of timing issues. If the tests are relying on file system operations, there might be timing-related issues that are causing the flakiness. Try adding some delays or retries to the tests to see if that improves the situation.
4. Look for race conditions. The changes introduce multithreading in some tests. Ensure there are no race conditions when accessing shared resources like the file system.

Without access to the test logs, it's difficult to pinpoint the exact cause of the flakiness. However, by focusing on the file path handling logic and the new test case, you should be able to narrow down the possibilities and identify the root cause.

---

Is this helpful? Click the 👍/👎 buttons below to let us know!

Icons by Icons8

[trunk-staging-io]

     

View Full Report ↗︎ ⋅ Docs

[trunk-staging-io]

     

Failed Test	Failure Summary	Logs
test::quarantining_not_active_when_disable_true_but_use_true	The test failed because a file already exists, which caused a 'File exists' error when trying to create a new file.	Logs ↗︎
upload::test_variant_propagation	The file creation failed because the file already exists.	Logs ↗︎
upload::upload_bundle	The file upload failed because the file already exists.	Logs ↗︎
test::test_command_fails_with_upload_not_connected	An upload operation failed because a file already exists.	Logs ↗︎

... and 15 more

View Full Report ↗︎ ⋅ Docs

[trunk-io]

     

View Full Report ↗︎ ⋅ Docs

[gnalh]

Same comment here, intentional?

[gnalh]

same here.

[gnalh]

This will generate a new test case id for customers. We need to not have the /.

[cmillar-trunk]

Whoop, these are a result of a bad regex when I reverted the absolute path logic. Fixed.

[gnalh]

Similar comments here, we should not be modifying the file value in these cases (as they will generate new test ids).

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 96.91358% with 15 lines in your changes missing coverage. Please review.

Project coverage is 74.06%. Comparing base (aa7df72) to head (ddc33bd).

Files with missing lines	Patch %	Lines
cli/src/validate_command.rs	0.00%	6 Missing ⚠️
context/src/junit/file_extractor.rs	99.14%	3 Missing ⚠️
cli/src/context.rs	33.33%	2 Missing ⚠️
junit-mock/src/main.rs	0.00%	2 Missing ⚠️
context-js/src/lib.rs	0.00%	1 Missing ⚠️
context-py/src/lib.rs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #616      +/-   ##
==========================================
+ Coverage   73.19%   74.06%   +0.87%     
==========================================
  Files          69       70       +1     
  Lines       15690    16130     +440     
==========================================
+ Hits        11484    11947     +463     
+ Misses       4206     4183      -23     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[gnalh]

I've been thinking through this logic a bit more. I'm wondering if it would be good to store this simplified file path in a separate place. One concern I have is with accidentally generating new test IDs. Modifying file path means that we end up modifying those IDs and lose test history. If we keep the original file and instead store the simplified path separately, we can primarily use it for codeowners + visibility in the UI and it would remove that concern.

[cmillar-trunk]

I've been thinking through this logic a bit more. I'm wondering if it would be good to store this simplified file path in a separate place. One concern I have is with accidentally generating new test IDs. Modifying file path means that we end up modifying those IDs and lose test history. If we keep the original file and instead store the simplified path separately, we can primarily use it for codeowners + visibility in the UI and it would remove that concern.

I feel like that runs counter to what our original goal was. Our original goal was to handle vitest's annoying habit of:

• putting test files in the classname attribute, and
• making test files relative to their package.json

If we don't do the filepath work, it's really easy to mistake the classname attribute (since every other framework uses it to store, uh, class names), so we'd have to try and find a file that matches the classname.

All in all, if we want to minimise the risk of changing files, I could change this logic to use the classname if and only if the usual file attributes aren't present AND we can find a path relative to a package.json file that matches the classname. Then completely drop the absolutification of relative paths.

[gnalh]

I've been thinking through this logic a bit more. I'm wondering if it would be good to store this simplified file path in a separate place. One concern I have is with accidentally generating new test IDs. Modifying file path means that we end up modifying those IDs and lose test history. If we keep the original file and instead store the simplified path separately, we can primarily use it for codeowners + visibility in the UI and it would remove that concern.

I feel like that runs counter to what our original goal was. Our original goal was to handle vitest's annoying habit of:

• putting test files in the classname attribute, and
• making test files relative to their package.json

If we don't do the filepath work, it's really easy to mistake the classname attribute (since every other framework uses it to store, uh, class names), so we'd have to try and find a file that matches the classname.

All in all, if we want to minimise the risk of changing files, I could change this logic to use the classname if and only if the usual file attributes aren't present AND we can find a path relative to a package.json file that matches the classname. Then completely drop the absolutification of relative paths.

I should have been more explicit here. I'm not suggesting putting this in classname. I'm suggesting we store it somewhere else completely. We would keep classname, file, etc as is, but also have a new field called something like "detected_file". This would live in the internal_bin representation and would be used for display / code owners reason.

[cmillar-trunk]

I should have been more explicit here. I'm not suggesting putting this in classname. I'm suggesting we store it somewhere else completely. We would keep classname, file, etc as is, but also have a new field called something like "detected_file". This would live in the internal_bin representation and would be used for display / code owners reason.

I see, you're proposing we start by just detecting the file and putting it in a separate field, then as separate work use that to display/validate?

[gnalh]

I should have been more explicit here. I'm not suggesting putting this in classname. I'm suggesting we store it somewhere else completely. We would keep classname, file, etc as is, but also have a new field called something like "detected_file". This would live in the internal_bin representation and would be used for display / code owners reason.

I see, you're proposing we start by just detecting the file and putting it in a separate field, then as separate work use that to display/validate?

Exactly. That way we have a much higher tolerance for getting this wrong. It is a "best guess" after all.