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

Introduce --sim.exclude flag for excluding suites / tests #737

Open
Rjected opened this issue Mar 2, 2023 · 4 comments
Open

Introduce --sim.exclude flag for excluding suites / tests #737

Rjected opened this issue Mar 2, 2023 · 4 comments

Comments

@Rjected
Copy link
Contributor

Rjected commented Mar 2, 2023

While --sim.limit is useful, we have been setting up some hive suites to run as github actions, and some test suites are not yet applicable, and should be excluded.

For example, snap in the devp2p simulator is not yet applicable because we do not support the snap subprotocol yet. The sim.limit does accept a regular expression, but the golang regular expression library does not support negative lookaheads / lookbehinds. To exclude a suite, we have to explicitly include every other suite. To exclude a test, we have to explicitly include every other test.

To exclude snap, we have to run --sim.limit for each suite we want to include in devp2p, which is luckily only discv4 and eth.

I think it makes sense to allow both --sim.limit and --sim.exclude for this use case, for example:

hive --clients reth --sim devp2p --sim.limit eth --sim.exclude /TestBlockHashAnnounce

Because reth does not process block hash announcements, this would exclude the right test, while allowing us to run all other tests in eth. Without sim.exclude, we would have to enumerate all tests we want to include using the sim.limit flag.

Does this make sense?

@spencer-tb
Copy link
Contributor

Will take a look into adding this over the next couple days. I think should be simple enough:

  1. Adding another flag.
  2. Copying the regex logic for the flag with a not: !re.MatchString(...)).
  3. Adding additional logic for the combination of both limit and exclude.

@Rjected
Copy link
Contributor Author

Rjected commented Jun 25, 2024

Will take a look into adding this over the next couple days. I think should be simple enough:

1. Adding another flag.

2. Copying the regex logic for the flag with a not: `!re.MatchString(...))`.

3. Adding additional logic for the combination of both limit and exclude.

hey @spencer-tb, any progress on this?

@spencer-tb
Copy link
Contributor

Sorry for the delay here, I started on this locally but never quite finalized everything. Some updates nonetheless!

I'm currently using this branch: spencer-tb/sim-exclude-flag

Usage and semantics so far are below:

--sim.limit "<suite_inclusion_pattern>/<test_inclusion_pattern>" --sim.omit "<suite_exclusion_pattern>/<test_exclusion_pattern>"

Note that exclusion is checked first. So if there is an overlap between inclusion and exclusion the test or suite will always be skipped.

I have the functionality working for some simulators, i.e ethereum/pyspec, ethereum/engine, however as the test limiting is different for devp2p and rpc-compat (simulators I am not familiar with) I'll need to update them for the exclusion to work.

Nonetheless the first feature should work for the devp2p simulator. That is if you want to only run the discv4 and eth suites you can use --sim.omit "snap|discv5/". Although that is equivalent to running --sim.limit "eth|discv4/" so there is not much point to this for devp2p as you mention.

@spencer-tb
Copy link
Contributor

spencer-tb commented Jul 12, 2024

I will update you once I have finalized everything to work across all simulators. :)

@Rjected

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

No branches or pull requests

2 participants