fix(forge) hotfix - filter tests #5822

iFrostizz · 2023-09-12T15:07:42Z

Hotfix for #5753
Closes #5563

I have introduced a bug that didn't filtered the tests correctly using the construct.
Fixes and ~slight refactor so that won't repeat cause we don't depend on 2 instance of the same code.
Also we don't need to turn off debug traces, what I initially thought was a big performance penalty was just proptest shrinking while it wasn't ran in --release mode

Evalir

lgtm, are we sure this fixes filtering? is anything else broken right now?

Evalir

needs fixing on tests—let's make sure this works very well before merging

iFrostizz · 2023-09-12T15:21:58Z

Yeah @Evalir , forge debug is broken with "Invalid hex calldata" but I'm wondering if we shouldn't just nuke it at this point, c.f. #4793 (comment)
tldr it serves the same purpose as forge script --debug and is just redundant
You can see that it's forge script with less stuff, it's even calling run_script soooo https://github.com/foundry-rs/foundry/blob/master/crates/forge/bin/cmd/debug.rs#L43-L56

Evalir · 2023-09-12T15:23:18Z

@iFrostizz happy to jettison it but let's do it in another PR—don't wanna mix a hotfix with removing a feature :) one thing at a time

iFrostizz · 2023-09-12T15:42:28Z

Yep @Evalir , that was a blunder introduced in #5692
We should probably write some tests at a later point that make sure that the debugger opens (no crash) with the right breakpoints so these would have been caught on CI

iFrostizz · 2023-09-12T15:51:10Z

So just to make it clear is it okay if forge debug is currently broken ? Do we have a consensus to drop it ? (personally in favor of doing it)
If it's a yes, TODO: do lots of changes in the docs, where forge debug is mentioned and mostly replace by forge script --debug (it's almost a drop-in replacement, which is why is makes sense to drop it) as well as in the forge commands.

Evalir · 2023-09-12T16:31:24Z

hmm, i think it requires a bit of discussion with the community as well as this is technically a breaking change—is it possible to fix up forge debug first? @iFrostizz

mattsse · 2023-09-12T16:47:12Z

can you elaborate on what is broken and why dropping forge debug is a good idea from your pov? @iFrostizz

iFrostizz · 2023-09-12T17:43:50Z

@Evalir good idea, let's fix this stuff first
@mattsse coming from #4793 (comment) . forge debug is basically a slimmed down version of forge script --debug because it has much less options and hardcode stuffs just to satisfy the "script" part. It a big footgun imo, just like the scenario of the guy linked before. Because people may think that their script cannot be debugged while they should just use forge script --debug.
For legacy reasons, we could just copy the behavior of forge script --debug over forge debug, not sure if clap can flatten structs and hide fields from the command line conditionally cause a new struct can be created by composition not inheritance in Rust. dunno

iFrostizz · 2023-09-12T18:43:33Z

We can just flatten DebugArgs with [clap(flatten)] and make ScriptArgs composed from DebugArgs

Evalir · 2023-09-12T19:28:22Z

@iFrostizz i believe that should be good enough—will we include this in this PR?

Evalir

Thanks for the fix! Can you confirm you've QA'd this and works?

Also, unrelated, but I'd recommend enabling git commit signing—it's more secure and a good practice.

iFrostizz · 2023-09-12T19:31:35Z

@Evalir just dropped the stuff, let's see how you find it.
Sorry for the pgp stuff, just switched to a new DE, need to bring it back in a min

iFrostizz · 2023-09-12T19:43:20Z

iFrostizz · 2023-09-12T20:06:22Z

The last commit just makes ScriptArgs "inherits" (it's composed from) DebugArgs with the only diff that debug is hardcoded to true for DebugArgs. Keeping it for legacy purposes, but you can see how duplicated that command feels like. In short it's forge script without the --debug flag.

crates/forge/bin/cmd/debug.rs

Evalir · 2023-09-21T15:14:54Z

friendly ping @iFrostizz

This reverts commit f1af410.

iFrostizz · 2023-09-21T16:12:10Z

Yep, sorry @Evalir !

iFrostizz · 2023-09-21T16:18:20Z

So now, forge script --debug ARGS is always equivalent to forge debug ARGS

iFrostizz · 2023-09-21T16:29:08Z

wait fuzzing is still broken for forge debugand forge script --debug

iFrostizz · 2023-09-21T16:51:21Z

@Evalir I get it, script's --sig flag is not supposed to be ran against any argument-based function, so no fuzzing for scripts, makes sense yeah ?
LGTM otherwise

Evalir · 2023-09-21T20:26:19Z

Yep, I think this makes sense—no reason why would you fuzz in a script!

thank you, will review soon!

iFrostizz · 2023-09-25T15:55:18Z

@mattsse friendly ping
Making sure you saw that review request

mattsse

I have introduced a bug that didn't filtered the tests correctly using the construct.

I want to do these always in two steps, fix first then refactor.

this is not really easy to review because unclear what the fix is and what refactor.

mattsse · 2023-09-25T16:51:23Z

crates/forge/bin/cmd/debug.rs

+    /// Max priority fee per gas for EIP1559 transactions.
+    #[clap(
+        long,
+        env = "ETH_PRIORITY_GAS_PRICE",
+        value_parser = parse_ether_value,
+        value_name = "PRICE"
+    )]
+    pub priority_gas_price: Option<U256>,
+
+    /// Use legacy transactions instead of EIP1559 ones.
+    ///
+    /// This is auto-enabled for common networks without EIP1559.
    #[clap(long)]
-    pub debug: bool,
+    pub legacy: bool,
+
+    /// Broadcasts the transactions.
+    #[clap(long)]
+    pub broadcast: bool,
+
+    /// Skips on-chain simulation.
+    #[clap(long)]
+    pub skip_simulation: bool,
+
+    /// Relative percentage to multiply gas estimates by.
+    #[clap(long, short, default_value = "130")]
+    pub gas_estimate_multiplier: u64,


I'm having trouble understanding why changing all these fields is necessary in order to fix the referenced issue?

mattsse · 2023-09-25T16:55:24Z

I have introduced a bug that didn't filtered the tests correctly using the construct.

this PR should then fix this bug,

could you please split this into two, because rn it looks like this pr does multiple things

mattsse · 2023-09-25T17:00:51Z

but maybe @Evalir has already checked it, in which case I'm okay with merging, codewise this looks fine, but the impl figment macro is now no longer accurate

Evalir · 2023-09-25T17:02:19Z

@mattsse im feeling uneasy about this PR due to the same reasons you mentioned—I'm in favor of splitting to make changes smaller and ideally also not screw up the figment macro

Evalir · 2023-11-01T19:33:17Z

Closing, this was fixed recently in another PR.

iFrostizz added 2 commits September 12, 2023 14:55

factor filtered_tests function

c0f0067

keep debug traces on for fuzz

e8accec

Evalir approved these changes Sep 12, 2023

View reviewed changes

Evalir requested changes Sep 12, 2023

View reviewed changes

add filtering method

e139cb5

iFrostizz requested a review from Evalir September 12, 2023 15:53

Evalir approved these changes Sep 12, 2023

View reviewed changes

ScriptArgs composes from DebugArgs

f1af410

iFrostizz requested a review from Evalir September 12, 2023 19:32

Evalir requested changes Sep 12, 2023

View reviewed changes

crates/forge/bin/cmd/debug.rs Show resolved Hide resolved

iFrostizz added 2 commits September 21, 2023 17:50

Revert "ScriptArgs composes from DebugArgs"

2b820fe

This reverts commit f1af410.

Write ScriptArgs into DebugArgs

1c49e07

iFrostizz requested a review from Evalir September 21, 2023 16:12

Evalir requested a review from mattsse September 21, 2023 20:26

mattsse requested changes Sep 25, 2023

View reviewed changes

Evalir closed this Nov 1, 2023

zerosnacks mentioned this pull request Jul 3, 2024

forge test --debug fails with Invalid hex calldata when invoked on tests that take fuzz arguments #5563

Closed

2 tasks

fix(forge) hotfix - filter tests #5822

fix(forge) hotfix - filter tests #5822

Uh oh!

Conversation

iFrostizz commented Sep 12, 2023

Uh oh!

Evalir left a comment

Choose a reason for hiding this comment

Uh oh!

Evalir left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iFrostizz commented Sep 12, 2023

Uh oh!

Evalir commented Sep 12, 2023

Uh oh!

iFrostizz commented Sep 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iFrostizz commented Sep 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Evalir commented Sep 12, 2023

Uh oh!

mattsse commented Sep 12, 2023

Uh oh!

iFrostizz commented Sep 12, 2023

Uh oh!

iFrostizz commented Sep 12, 2023

Uh oh!

Evalir commented Sep 12, 2023

Uh oh!

Evalir left a comment

Choose a reason for hiding this comment

Uh oh!

iFrostizz commented Sep 12, 2023

Uh oh!

iFrostizz commented Sep 12, 2023

Uh oh!

iFrostizz commented Sep 12, 2023

Uh oh!

Uh oh!

Evalir commented Sep 21, 2023

Uh oh!

iFrostizz commented Sep 21, 2023

Uh oh!

iFrostizz commented Sep 21, 2023

Uh oh!

iFrostizz commented Sep 21, 2023

Uh oh!

iFrostizz commented Sep 21, 2023

Uh oh!

Evalir commented Sep 21, 2023

Uh oh!

iFrostizz commented Sep 25, 2023

Uh oh!

mattsse left a comment

Choose a reason for hiding this comment

Uh oh!

mattsse Sep 25, 2023

Choose a reason for hiding this comment

Uh oh!

mattsse commented Sep 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattsse commented Sep 25, 2023

Uh oh!

Evalir commented Sep 25, 2023

Uh oh!

Evalir commented Nov 1, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Evalir left a comment •

edited

Loading

iFrostizz commented Sep 12, 2023 •

edited

Loading

iFrostizz commented Sep 12, 2023 •

edited

Loading

mattsse commented Sep 25, 2023 •

edited

Loading