Thank you for working on improving the testing infra!
I'd suggest looking into the already-existing native python functionality: https://github.com/freebsd/freebsd-src/blob/main/tests/examples/test_examples.py#L86
It would be nice to use something better than a shell for complex test scenarios - that would simplify the reusability and reduce the amount of code.
What do you think?

There's certainly a lot of repetition in many of the pf (and other firewall) tests.

So far I've chosen to not do anything about that, because it makes each individual test case much easier to understand. Each test case fully describes the setup it operates in, and when someone tries to debug it (or understand it for any other reason) there's no need to go look at other files to work out what the setup actually is.

I'm not 100% committed to this approach, but at the very least I'd like to see examples of what the tests themselves end up looking like.

In D38126#866273, @kp wrote:

There's certainly a lot of repetition in many of the pf (and other firewall) tests.

So far I've chosen to not do anything about that, because it makes each individual test case much easier to understand. Each test case fully describes the setup it operates in, and when someone tries to debug it (or understand it for any other reason) there's no need to go look at other files to work out what the setup actually is.

I'm not 100% committed to this approach, but at the very least I'd like to see examples of what the tests themselves end up looking like.

Sure!
https://github.com/freebsd/freebsd-src/blob/main/tests/sys/netlink/test_rtnl_ifaddr.py -- probably the most clear one

https://github.com/freebsd/freebsd-src/blob/main/tests/sys/net/routing/test_rtsock_multipath.py -- tests with high level of parametrisation
https://github.com/freebsd/freebsd-src/blob/main/tests/sys/netinet6/test_ip6_output.py -- 2-jails tests with the separate python handler

In D38126#866273, @kp wrote:

There's certainly a lot of repetition in many of the pf (and other firewall) tests.

So far I've chosen to not do anything about that, because it makes each individual test case much easier to understand. Each test case fully describes the setup it operates in, and when someone tries to debug it (or understand it for any other reason) there's no need to go look at other files to work out what the setup actually is.

I must say that I found the old code quite hard to understand, not due to repetition but due to jail and variable names not really reflecting what they do. That might work fine for just a single jail with a single epair interface, but once you get a router and a server it became a bit messy to my eyes.

I'm not 100% committed to this approach, but at the very least I'd like to see examples of what the tests themselves end up looking like.

The use case for those changes is in D38129. I thought splitting the huge amount of changes in tests I've ended up with while developing the OpenBSD-like scrub syntax into separate reviews would be a good idea, but I think it backfired. Should we merge those to reviews into one?

In D38126#866296, @vegeta_tuxpowered.net wrote:

In D38126#866273, @kp wrote:

I'm not 100% committed to this approach, but at the very least I'd like to see examples of what the tests themselves end up looking like.

The use case for those changes is in D38129. I thought splitting the huge amount of changes in tests I've ended up with while developing the OpenBSD-like scrub syntax into separate reviews would be a good idea, but I think it backfired. Should we merge those to reviews into one?

That, along with the code changes themselves, are on my list, but I don't expect to get the serious time and concentration that'll require before March.

! In D38126#866510, @kp wrote:

! In D38126#866296, @vegeta_tuxpowered.net wrote:

The use case for those changes is in D38129. I thought splitting the huge amount of changes in tests I've ended up with while developing the OpenBSD-like scrub syntax into separate reviews would be a good idea, but I think it backfired. Should we merge those to reviews into one?

That, along with the code changes themselves, are on my list, but I don't expect to get the serious time and concentration that'll require before March.

If it's gonna take so long, I could in the meantime use the routing scenarios proposed in this review and pick tests for the current pf.conf syntax from D38129. Maybe with the amount of tests for all scrub and rtable functions I could convince you to the benefits of splitting creation of the testing environment into reusable functions :) D38129 would then be left only with tests for the new OpenBSD-compatible syntax and could be reviewed together with it later.

In D38126#866510, @kp wrote:

In D38126#866296, @vegeta_tuxpowered.net wrote:

In D38126#866273, @kp wrote:

I'm not 100% committed to this approach, but at the very least I'd like to see examples of what the tests themselves end up looking like.

The use case for those changes is in D38129. I thought splitting the huge amount of changes in tests I've ended up with while developing the OpenBSD-like scrub syntax into separate reviews would be a good idea, but I think it backfired. Should we merge those to reviews into one?

That, along with the code changes themselves, are on my list, but I don't expect to get the serious time and concentration that'll require before March.

Would you be open to reviewing python re-implementation of some of the existing pf tests?

In D38126#866663, @melifaro wrote:

Would you be open to reviewing python re-implementation of some of the existing pf tests?

I am skeptical that that'd be an improvement for most of the current pf tests, but open to being proved wrong. Maybe pick one that you think would improve the most? (i.e. one, to have something specific to discuss.)

In D38126#866648, @vegeta_tuxpowered.net wrote:

If it's gonna take so long, I could in the meantime use the routing scenarios proposed in this review and pick tests for the current pf.conf syntax from D38129. Maybe with the amount of tests for all scrub and rtable functions I could convince you to the benefits of splitting creation of the testing environment into reusable functions :) D38129 would then be left only with tests for the new OpenBSD-compatible syntax and could be reviewed together with it later.

Yeah, that probably makes sense. It'd also be good to separate the "remove redundancy" and "adjust for new syntax" changes into separate commits.

Can you point me at the review for the setup_router_dummy_ipv4, ping_server_check_reply, .. functions? I'm sure you posted that somewhere, but I can't seem to find it.

I'm certainly seeing the argument for having setup_router_dummy_ipv6 and friends do the test setup.

In D38126#869129, @kp wrote:

I'm certainly seeing the argument for having setup_router_dummy_ipv6 and friends do the test setup.

I've generated the diff wrongly. Now it's fixed.

Yeah, this is a clear improvement.

I do think we need to document what the setup functions do, and what variables they make available. Perhaps in a comment block on top of them?