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

Fixes #7350. Added support for absolute paths for "forge test --match-path" #7362

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

Conversation

matthewliu10
Copy link

@matthewliu10 matthewliu10 commented Mar 11, 2024

Motivation

Related to #7350 .

forge test --match-path absolute_path will report No tests match the provided pattern, only relative path works.

--match-path should work for both absolute path and relative path.

Solution

If the path given by the user is absolute, and the path given to the function is relative, it uses the canonicalize() function to turn the path given to the function into an absolute path so that the paths can be compared.

Notes

I initially tried converting both paths into their canonical forms using canonicalize and directly comparing them, however this failed on the can_test_with_match_path test because it would attempt to canonicalize "*src/ATest.t.sol" which causes an error.

There may be cases where the paths refer to the same location but is_match() still returns false. For example, if the user inputs "/home/user/a/b/../..//c/", it may not match with "/home/user/c/" even though they are equivalent. Note: canonicalize() resolves any symbolic links, relative path components, and removes any redundant components in the path.

Tests

The following 5 tests fail when running cargo test --all --all-features:

cmd::can_install_missing_deps_build
cmd::can_install_missing_deps_test
cmd::can_update_library_with_outdated_nested_dependency
config::can_parse_default_fs_permissions
ext_integration::snekmate

However, the following 4 tests still failed when I reverted the changes that I made (reverted code to the original code that I pulled from the repo):

cmd::can_install_missing_deps_build
cmd::can_install_missing_deps_test
cmd::can_update_library_with_outdated_nested_dependency
ext_integration::snekmate

The following test failed when I ran cargo test --all --all-features, but passed when I ran it individually:

config::can_parse_default_fs_permissions

@matthewliu10 matthewliu10 force-pushed the bugfix/issue-7350-forge-test-absolute-path branch from 8bb3001 to f3a9674 Compare March 11, 2024 14:48
@mattsse mattsse requested a review from klkvr March 11, 2024 16:45
@matthewliu10
Copy link
Author

The test that is failing is the new one that I added. When testing locally, it is passing:
image

crates/common/src/glob.rs Outdated Show resolved Hide resolved
@matthewliu10
Copy link
Author

I've just run the failing tests locally. Every test either fails when using the code from the repo, or passes when using both the code from the repo and the new code:

forge::cli config::can_prioritise_closer_lib_remappings (fails with original code and new code)
forge::cli create::can_create_using_unlocked (fails with original code)
forge::cli config::can_detect_lib_foundry_toml (fails with original code)
forge::cli config::can_override_config (fails with original code)
forge::cli create::can_create_with_constructor_args (fails with original code)
forge::cli cmd::can_init_vscode (fails with original code)
anvil::it traces::test_trace_address_fork (passes with original code and new code)
anvil::it fork::test_fork_call (passes with original code and new code)
anvil::it fork::test_fork_timestamp (passes with original code and new code)
anvil::it otterscan::can_call_ots_get_internal_operations_contract_selfdestruct (passes with original code and new code)
anvil::it traces::test_trace_address_fork2 (passes with original code and new code)
forge::cli create::can_create_template_contract (fails with original code)
chisel::cache test_write_session (passes with original code and new code)

@klkvr
Copy link
Member

klkvr commented Mar 12, 2024

yeah, the ci is a little bit broken right now after forge-std release, we will fix this soon

@mattsse
Copy link
Member

mattsse commented Mar 12, 2024

is fixed on master

@matthewliu10
Copy link
Author

I just synced it again

@matthewliu10
Copy link
Author

I just ran the following tests locally before and after syncing with master. They all passed with both versions.

forge::cli script::can_broadcast_script_skipping_simulation
forge::cli test_cmd::can_disable_block_gas_limit
anvil::it traces::test_trace_address_fork
anvil::it fork::test_fork_timestamp

@matthewliu10
Copy link
Author

Please review when you get the chance

@DaniPopes DaniPopes requested review from klkvr, mattsse and DaniPopes and removed request for mattsse and DaniPopes May 24, 2024 20:39
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

3 participants