--match-contract and --match-test changed to glob pattern #3916

jatkz · 2022-12-20T02:10:19Z

Motivation

Solution

Patterns take a comma separated string which converts to a vector of globs. Jaro Winkler got lowered a little because glob symbols mess with prefix.

edit: I rebased a few times so its cleaner when it gets reviewed the first time, i did some refactoring.

refac and fmt

mattsse

thanks for taking this on.

love the test.

I have some concerns about the leading *.

Is this now required when trying to match parts of the name?

This would be breaking behavior.

could you provide some examples of how matching will change with this PR?

mattsse · 2022-12-21T13:47:39Z

cli/tests/it/test_cmd.rs

@@ -71,8 +70,8 @@ contract Dummy {}
        .unwrap();

    // set up command
-    cmd.args(["test", "--match-test", "testA.*", "--no-match-test", "testB.*"]);
-    cmd.args(["--match-contract", "TestC.*", "--no-match-contract", "TestD.*"]);
+    cmd.args(["test", "--match-test", "*testA*", "--no-match-test", "*testB*"]);


is the leading * now necessary?

jatkz · 2022-12-21T14:20:26Z

thanks for taking this on.

love the test.

I have some concerns about the leading *.

Is this now required when trying to match parts of the name?

Ohh, you are right. It is a silly mistake on my part.

I tested without the leading * and it works fine. I can go through and fix everything this evening.

Thank you for your review

mattsse · 2022-12-22T11:30:33Z

@mds1 wdyt?

I think this is an overall improvement but is technically breaking, right? since no more regex. however this is more in line with how other tools do test matching via cli

gakonst · 2022-12-22T12:38:09Z

I believe people use some regex syntax to exclude stuff, do we still support this here?

mds1 · 2022-12-22T14:01:48Z

Will look soon! @jatokz Do you mind leaving a comment here with some examples of what works / what doesn't work for each matcher (contract, test, path), compared to what did/didn't work before?

From reading the conversation here my main concern would be removing regex support, as I do think that's useful (and have found it useful myself). With good naming conventions + regex it's easy to target only certain test functions during development, so I want to better understand the tradeoffs of this PR

jatkz · 2022-12-23T00:26:57Z

I believe people use some regex syntax to exclude stuff, do we still support this here?

in this pr, the exclude patterns are also changed to comma separated globs

@mds1 sure! made some progress, going to spend some time to try and get a good list

gakonst · 2022-12-25T12:37:34Z

Thanks king - appreciate the effort

jatkz · 2022-12-26T03:50:49Z

hope everyone had a good holiday

@mds1 I came up with a few examples:

Case Sensitive:
testComputeCreate2Address
test_createAdress

glob => *[cC]reate*
re => .*[cC]reate.* OR (?i)(.*create.*)

Match where contains Set or Update
testSetFoo
testUpdateBar

glob => *Set*,*Update*
re => .*(Set|Update).*

Contain both words in any order

glob => doesnt seem possible with this lib so would have to do a comma sep list of every combination
re => (?=.*Foo)(?=.*Bar).*

Both words in order

glob => *Foo*Bar*
re => .*Foo.*Bar.*

Negation in front without using exclude param

testFooDooBar - No
testDooBar - yes

glob => *[!Foo]Bar* <= mismatch only if foo in front
re => ^((?!Foo).)*Bar <= mismatch if foo in there at all, im sure there is a way to do it more specific

there are some other globs patterns available here: https://docs.rs/glob/latest/glob/struct.Pattern.html

mds1 · 2022-12-27T15:22:53Z

Thanks for those comparisons, that was helpful!

Seems simple enough of a change UX-wise. In general I think I'm indifferent on globs vs. regex, and since globs can accomplish pretty much the same thing, this lgtm. It may be worth including something similar to those comparisons in the book too, to help users migrate.

@gakonst Seems this will be a breaking change that may break some scripts/workflows/CIs, so we should try to communicate this once merged since I suspect these are commonly used features

gakonst · 2022-12-27T16:22:18Z

SGTM - Can we document these in the book after we merge so that we can point people to a resource to modify their scripts?

jatkz · 2022-12-27T19:22:36Z

i linked a pr for the book, its kind of minimalist,

I can add the regex comparison as well if you think its good

gakonst · 2022-12-28T17:46:11Z

Yeah would love to get more examples in the book as well now that we're on it - go for it. Lgtm pending @mds1 approval

mds1 · 2022-12-29T16:53:04Z

I haven’t tested the PR or reviewed the code (prob won’t be able to until after the holidays) but conceptually it lgtm!

gakonst · 2023-01-03T13:28:18Z

@mds1 LMK when you get a chance to try!

mds1 · 2023-01-04T01:25:17Z

Testing with --mt and seems to be working. Two comments:

It only works if I put quotes around the glob, which is tedious. Can we make it work without quotes? For example forge test --mt test* finds nothing, but forge test --mt "test*" runs all tests
Currently it's case sensitive, which I think is consistent with the prior UX right?

jatkz · 2023-01-04T04:11:11Z

Yes, I'm going to try to get that fixed soon. edit: see below, from what I know I'm not sure if there is anyway around it?
I tested prior regex just now and it is consistent

jatkz · 2023-01-05T06:05:58Z

Update on 1) from testing on my machine, since I am using a bash shell the test* was being changed to test because it functioned like a glob in my shell and there was a test directory so it matched to that.

One way to verify that this is true for you also is to do forge test --mt test\* since that escapes into literal *.

It seems that will introduce a new issue that does not occur with regex?

I'm open to any additional changes that could improve further.

mds1 · 2023-01-09T18:13:54Z

Ah yes escaping with forge test --mt test\* does work. But it seems this is true even if there are no directory matches. For example testing on the forge-std repo I can't run forge test --mt *Storage* to run all storage-related tests. @mattsse @gakonst curious your thoughts on this?

gakonst · 2023-01-17T19:12:53Z

Would like to get this in a place where it doesn't require quotes or escapes, like it is currently..Can we not parse it as String from CLI, and bring it in the right form/global format internally?

zerosnacks · 2024-07-31T10:35:27Z

Is this PR still desired? Personally I prefer the regex pattern as it is much more expressive

Moving this outside of the 1.0 milestone as it is a breaking change

mds1 · 2024-07-31T15:10:33Z

I'd be ok closing this, since I haven't seen any complaints about the current behavior

jatkz changed the title ~~patterns changed to glob~~ --match-contract and --match-test changed to glob pattern Dec 20, 2022

jatkz force-pushed the globs branch 2 times, most recently from 71c091c to e315634 Compare December 21, 2022 02:21

patterns changed to glob

595e0f4

refac and fmt

jatkz force-pushed the globs branch from 1d2d225 to 595e0f4 Compare December 21, 2022 02:44

jatkz added 2 commits December 20, 2022 20:51

fix lint errors

83b510c

ran clippy locally and fix lint error

e654516

mattsse reviewed Dec 21, 2022

View reviewed changes

mattsse added T-feature Type: feature C-forge Command: forge labels Dec 21, 2022

removed unneeded leading * in tests and suggestion

8c6e601

jatkz mentioned this pull request Dec 27, 2022

regex to glob patterns foundry-rs/book#749

Open

zerosnacks modified the milestone: v1.0.0 Jul 31, 2024

zerosnacks added the Cmd-forge-test Command: forge test label Jul 31, 2024

zerosnacks closed this Jul 31, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

--match-contract and --match-test changed to glob pattern #3916

--match-contract and --match-test changed to glob pattern #3916

jatkz commented Dec 20, 2022 •

edited by zerosnacks

Loading

mattsse left a comment •

edited

Loading

mattsse Dec 21, 2022

jatkz commented Dec 21, 2022

mattsse commented Dec 22, 2022

gakonst commented Dec 22, 2022

mds1 commented Dec 22, 2022

jatkz commented Dec 23, 2022 •

edited

Loading

gakonst commented Dec 25, 2022

jatkz commented Dec 26, 2022 •

edited

Loading

mds1 commented Dec 27, 2022

gakonst commented Dec 27, 2022

jatkz commented Dec 27, 2022 •

edited

Loading

gakonst commented Dec 28, 2022

mds1 commented Dec 29, 2022

gakonst commented Jan 3, 2023

mds1 commented Jan 4, 2023

jatkz commented Jan 4, 2023 •

edited

Loading

jatkz commented Jan 5, 2023 •

edited

Loading

mds1 commented Jan 9, 2023

gakonst commented Jan 17, 2023

zerosnacks commented Jul 31, 2024

mds1 commented Jul 31, 2024

--match-contract and --match-test changed to glob pattern #3916

--match-contract and --match-test changed to glob pattern #3916

Conversation

jatkz commented Dec 20, 2022 • edited by zerosnacks Loading

mattsse left a comment • edited Loading

Choose a reason for hiding this comment

mattsse Dec 21, 2022

Choose a reason for hiding this comment

jatkz commented Dec 21, 2022

mattsse commented Dec 22, 2022

gakonst commented Dec 22, 2022

mds1 commented Dec 22, 2022

jatkz commented Dec 23, 2022 • edited Loading

gakonst commented Dec 25, 2022

jatkz commented Dec 26, 2022 • edited Loading

mds1 commented Dec 27, 2022

gakonst commented Dec 27, 2022

jatkz commented Dec 27, 2022 • edited Loading

gakonst commented Dec 28, 2022

mds1 commented Dec 29, 2022

gakonst commented Jan 3, 2023

mds1 commented Jan 4, 2023

jatkz commented Jan 4, 2023 • edited Loading

jatkz commented Jan 5, 2023 • edited Loading

mds1 commented Jan 9, 2023

gakonst commented Jan 17, 2023

zerosnacks commented Jul 31, 2024

mds1 commented Jul 31, 2024

jatkz commented Dec 20, 2022 •

edited by zerosnacks

Loading

mattsse left a comment •

edited

Loading

jatkz commented Dec 23, 2022 •

edited

Loading

jatkz commented Dec 26, 2022 •

edited

Loading

jatkz commented Dec 27, 2022 •

edited

Loading

jatkz commented Jan 4, 2023 •

edited

Loading

jatkz commented Jan 5, 2023 •

edited

Loading