ping: Add ATF-Python tests

[jlduran]

Summary

ping(8) is an old utility, which has received many changes and updates through the years. Some of these changes may have introduced small bugs, in part due to the lack of tests.

Attempt to remedy the current situation by introducing a way to easily add tests.

[asomers]

This looks like a good start. I'm not a fan of the methodize stuff, though. I think it makes the tests harder to read and harder to search. Is there a way to parameterize the tests without using methodize, even if it becomes slightly more verbose?

sbin/ping/tests/test_ping.py

541

What does ids do?

[melifaro]

Conceptually LGTM, please see some comments on the structure

sbin/ping/tests/test_ping.py

507

Each test consists of params, stdout and returncode.
Maybe it's worth encoding them directly as the test parameters?

e.g.

    @pytest.mark.parametrize(
        "params",
        [
            pytest.param(
                {
                    "args": "-4 -c1 -s56 -t1 localhost",
                    "returncode": 2,
                    "stdout": """\
PING 192.0.2.2 (192.0.2.2): 56 data bytes

--- 192.0.2.2 ping statistics ---
3 packets transmitted, 0 packets received, 100.0% packet loss
"""
                },
                id="ipandif",
            ),
        ],
    )

546

@pytest.mark.timeout(10)

[jlduran]

In D38053#864580, @asomers wrote:

This looks like a good start. I'm not a fan of the methodize stuff, though. I think it makes the tests harder to read and harder to search. Is there a way to parameterize the tests without using methodize, even if it becomes slightly more verbose?

I'll look into it. The reasoning was a two-fold:

1. To avoid spaces inside parameters, test_ping[_Wx_localhost] instead of test_ping[-Wx localhost], having to escape spaces when debugging a particular test may not be desired, partly inspired by current ping_test.sh names.
2. To use them as fixtures, they need a proper method name.

But, yes... I'll try other options so we can streamline the creation of simple tests, maybe "inlining" the fixtures, as @melifaro is suggesting.

sbin/ping/tests/test_ping.py

541

The ids is the text inside the brackets when parameterized, (as in kyua list).
From https://docs.pytest.org/en/7.2.x/how-to/fixtures.html:

Numbers, strings, booleans and None will have their usual string representation used in the test ID. For other objects, pytest will make a string based on the argument name. It is possible to customize the string used in a test ID for a certain fixture value by using the ids keyword argument.

[jlduran]

sbin/ping/tests/test_ping.py

507

Yes, that's how they started, it looked bulky, but let me update it the way you are suggesting.
May I also add stderr (see the`_Wx_localhost`), to keep it inline with atf-check(1).
I also do not like that things are in "two places".

546

OK, will do. atf-check(1) uses 50 ms by default, subprocess.run() uses None.

[jlduran]

Address some of the suggestions:

• Have the parameters in a single location. Each parameter consists of a tuple containing the arguments to the ping command and an ExpectedProcess() (I prefer the approach of a custom class that mimics the structure of a subprocess.CompletedProcess() over a dictionary, however, I am open to suggestions).
• Set the timeout to 15 seconds, as some of the "slow" tests take a little over 13 seconds.

Next step, try to adapt D37876.

[melifaro]

Thank you for addressing the comments!
I’d still prefer to have the ids embedded explicitly via pytest.param, but I don’t insist on doing it.

Generally there may be somewhat conflicting ideas on what’s the “best” approach here, but personally I don’t think it’s a big deal here. This is a test code, it already is reasonably simple and allow for easy extension or debugging - that’s “good enough” and that’s what matters most (to me).

sbin/ping/tests/test_ping.py

65

I’ve some plans on checking inet/inet6 support and skip tests that require something that’s not supported.
This works iff the test requests only the required families. For some tests one can tweak the PREFIXES lis based on the test name, like here: https://github.com/freebsd/freebsd-src/blob/main/tests/sys/net/routing/test_rtsock_multipath.py#L10

Happy to discuss any approaches here

[jlduran]

Address more suggestions:

• Embed the ids explicitly via pytest.param().

[jlduran]

In D38053#864674, @melifaro wrote:

Thank you for addressing the comments!
I’d still prefer to have the ids embedded explicitly via pytest.param, but I don’t insist on doing it.

Generally there may be somewhat conflicting ideas on what’s the “best” approach here, but personally I don’t think it’s a big deal here. This is a test code, it already is reasonably simple and allow for easy extension or debugging - that’s “good enough” and that’s what matters most (to me).

Done! I wanted to avoid having to name things, but it is better to have control over it.

sbin/ping/tests/test_ping.py

65

Great! Yes, the idea is that it should be transparent to the consumer, and gracefully skip the test.

[jlduran]

Updates:

• Simple one-line ping parameter expectations are subprocess.CompletedProcesses. No need to have a custom class (ExpectedProcess) that does essentially the same thing.
• Incorporate the code from D37876. The pinger utility (now a function) returns a subprocess.CompletedProcess as well. Parametrization and cleanup are next.

[jlduran]

Here are the last iteration changes:

• Use a dictionary for expectations — this keeps both ping and pinger tests inline. Comparing the expected with the actual subprocess.CompletedProcess was also not feasible. It also was suggested initially by @melifaro.

One thing I noticed is that atf-python tests are slower than atf-sh, this was somewhat expected, but when all tests run, the total time builds up. I believe the gains are really from the development perspective.

We are now at a point where I feel OK to start adding more tests and actually fixing the ping utility.

Any suggestions, as always, are welcomed.

[melifaro]

In D38053#865132, @jlduran_gmail.com wrote:

Here are the last iteration changes:

• Use a dictionary for expectations — this keeps both ping and pinger tests inline. Comparing the expected with the actual subprocess.CompletedProcess was also not feasible. It also was suggested initially by @melifaro.

One thing I noticed is that atf-python tests are slower than atf-sh, this was somewhat expected, but when all tests run, the total time builds up. I believe the gains are really from the development perspective.

Q: what's the runtime? e.g. median time reported by kyua test?
There are some tests in net/routing, doing similar SingleVnet-isolated testing, written both in python and in C.
C version takes ~120ms per test, python version takes ~330ms. My take is that the runtime under 0.5 second is fine. It may also be not easy to decrease those 330ms, as pytest does a lot of preparation work, but the runner calls it for a single test.
I suspect that importing scapy.all may contribute to the delay here. Note that kyua needs to first run list for the tests (to determine the isolation details) and then actual test and cleanup procedures.
Importing scapy.all in the top of the file will cause all 3 cases to wait till scapy init. I'd probably try to check if it's possible to use scapy subset and/or try to load it only when needed.

We are now at a point where I feel OK to start adding more tests and actually fixing the ping utility.

Any suggestions, as always, are welcomed.

[jlduran]

In D38053#865288, @melifaro wrote:

In D38053#865132, @jlduran_gmail.com wrote:

Here are the last iteration changes:

• Use a dictionary for expectations — this keeps both ping and pinger tests inline. Comparing the expected with the actual subprocess.CompletedProcess was also not feasible. It also was suggested initially by @melifaro.

One thing I noticed is that atf-python tests are slower than atf-sh, this was somewhat expected, but when all tests run, the total time builds up. I believe the gains are really from the development perspective.

Q: what's the runtime? e.g. median time reported by kyua test?

atf-sh	atf-python
sbin/ping/ping_test:inject_opts -> passed [0.841s]	sbin/ping/test_ping.py:TestPing::test_pinger[_0_0_opts_NOP_40] -> passed [0.926s]
sbin/ping/ping_test:inject_pip -> passed [1.870s]	sbin/ping/test_ping.py:TestPing::test_pinger[_3_1_opts_NOP_40] -> failed: /usr/tests/sbin/ping/test_ping.py:889: AssertionError [2.160s]
sbin/ping/ping_test:inject_reply -> passed [0.826s]	sbin/ping/test_ping.py:TestPing::test_pinger[_0_0_opts_NOP_40] -> passed [0.926s]
sbin/ping/ping_test:ping_6_c1_s8_t1 -> passed [0.019s]	sbin/ping/test_ping.py:TestPing::test_ping[_6_c1_s8_t1_localhost] -> passed [1.217s]
sbin/ping/ping_test:ping_c1_s56_t1_S127 -> passed [0.028s]	sbin/ping/test_ping.py:TestPing::test_ping[_c1_S127_0_0_1_s56_t1_localhost] -> passed [1.337s]
sbin/ping/ping_test:ping_c1_s8_t1_S1 -> passed [0.026s]	sbin/ping/test_ping.py:TestPing::test_ping[_c1_S__1_s8_t1_localhost] -> passed [1.343s]

Source: https://cirrus-ci.com/task/5223802863353856?logs=test_ping

Notice how the last three tests take a second more. All python-based tests run inside a VNET, I wonder if there is something I'm missing that takes up a second more when pinging localhost. Or maybe subprocess.run() just waits too much?, I'll investigate.
The first three tests are scapy-based, given I prefer to test the entire stdout, I consider them within an acceptable margin.

There are some tests in net/routing, doing similar SingleVnet-isolated testing, written both in python and in C.
C version takes ~120ms per test, python version takes ~330ms. My take is that the runtime under 0.5 second is fine. It may also be not easy to decrease those 330ms, as pytest does a lot of preparation work, but the runner calls it for a single test.
I suspect that importing scapy.all may contribute to the delay here. Note that kyua needs to first run list for the tests (to determine the isolation details) and then actual test and cleanup procedures.
Importing scapy.all in the top of the file will cause all 3 cases to wait till scapy init. I'd probably try to check if it's possible to use scapy subset and/or try to load it only when needed.

Yes, in the worst scenario, I'll put those in a separate file, I'll try those suggestions. Thank you!

[melifaro]

So generally, it looks good to me, and I'm fine with committing the change. @asomers: what do you think?

sbin/ping/tests/test_ping.py

89

Nit: I'd wrap this in a function somewhere inside tests/atf_python/sys/net/tools.py or similar.
Setting sysctls is a common thing and we should abstract the implementation

[jlduran]

Thank you! I'll wait for @asomers comments to update the revision.

I'll document here my small - not really important - wishlist:

1. Optional randomization of the test order, maybe something similar to pytest-random-order.
2. When debugging atf-python tests, the output could be a little bit less busy, maybe pytest_terminal_summary et al. instead of print() could be one answer.
3. An atf_get_srcdir-equivalent to eventually read files relative to the source directory, however I think the idea is to disassociate from ATF later on.
4. In the meantime the kernel delayed object reclamation model issue is fixed, maybe adapt 80fc25025ffcb0d369fc0b6d4d272ad6fd3f53c3 ?

sbin/ping/tests/test_ping.py

89

Will do!

188

This can be abstracted as well, but I'll have to teach VnetInterface.setup_addr to understand tun(4) interfaces...

[melifaro]

In D38053#865570, @jlduran_gmail.com wrote:

Thank you! I'll wait for @asomers comments to update the revision.

I'll document here my small - not really important - wishlist:

1. Optional randomization of the test order, maybe something similar to pytest-random-order.

Nice one, can probably work out-of-the box, but the scope will be a single file, due to the way of ATF<>pytest interaction is implemented.

2. When debugging atf-python tests, the output could be a little bit less busy, maybe pytest_terminal_summary et al. instead of print() could be one answer.

I get the intent and I agree. Maybe you could come up with an example (or even a diff) on how do you see it?

3. An atf_get_srcdir-equivalent to eventually read files relative to the source directory, however I think the idea is to disassociate from ATF later on.

This one slipped through the cracks, thanks for reminding! Will add in a day or two.

4. In the meantime the kernel delayed object reclamation model issue is fixed, maybe adapt 80fc25025ffcb0d369fc0b6d4d272ad6fd3f53c3 ?

Yep, been on my list for quite some time, I'll do the change in a day or two.

[jlduran]

In D38053#866669, @melifaro wrote:

In D38053#865570, @jlduran_gmail.com wrote:

Thank you! I'll wait for @asomers comments to update the revision.

I'll document here my small - not really important - wishlist:

1. Optional randomization of the test order, maybe something similar to pytest-random-order.

Nice one, can probably work out-of-the box, but the scope will be a single file, due to the way of ATF<>pytest interaction is implemented.

2. When debugging atf-python tests, the output could be a little bit less busy, maybe pytest_terminal_summary et al. instead of print() could be one answer.

I get the intent and I agree. Maybe you could come up with an example (or even a diff) on how do you see it?

Sure! Once all ping tests are added, I should be more acquainted with the testing framework and propose a revision.

3. An atf_get_srcdir-equivalent to eventually read files relative to the source directory, however I think the idea is to disassociate from ATF later on.

This one slipped through the cracks, thanks for reminding! Will add in a day or two.

4. In the meantime the kernel delayed object reclamation model issue is fixed, maybe adapt 80fc25025ffcb0d369fc0b6d4d272ad6fd3f53c3 ?

Yep, been on my list for quite some time, I'll do the change in a day or two.

Great!

[ngie]

In D38053#864580, @asomers wrote:

This looks like a good start. I'm not a fan of the methodize stuff, though. I think it makes the tests harder to read and harder to search. Is there a way to parameterize the tests without using methodize, even if it becomes slightly more verbose?

If you’re referring to “pytest.marks.parametrize”, it supports an optional id= parameter which better describes tests for humans.

[jlduran]

In D38053#867487, @ngie wrote:

In D38053#864580, @asomers wrote:

This looks like a good start. I'm not a fan of the methodize stuff, though. I think it makes the tests harder to read and harder to search. Is there a way to parameterize the tests without using methodize, even if it becomes slightly more verbose?

If you’re referring to “pytest.marks.parametrize”, it supports an optional id= parameter which better describes tests for humans.

Yes, both @asomers and @melifaro suggested to not set the id programatically. Each parameter now has the id manually set. Thank you!

[melifaro]

3. An atf_get_srcdir-equivalent to eventually read files relative to the source directory, however I think the idea is to disassociate from ATF later on.

This one slipped through the cracks, thanks for reminding! Will add in a day or two.

I thought of passing it via wrapper initially, but ended up with a bit more natural way - getting it from pytest itself.
I committed an example here.

[melifaro]

4. In the meantime the kernel delayed object reclamation model issue is fixed, maybe adapt 80fc25025ffcb0d369fc0b6d4d272ad6fd3f53c3 ?

Committed in 20ea7f26e413 .

[jlduran]

In D38053#867921, @melifaro wrote:

3. An atf_get_srcdir-equivalent to eventually read files relative to the source directory, however I think the idea is to disassociate from ATF later on.

This one slipped through the cracks, thanks for reminding! Will add in a day or two.

I thought of passing it via wrapper initially, but ended up with a bit more natural way - getting it from pytest itself.
I committed an example here.

Ha, very nice! I tend to prefer the f-string format (i.e., print(f" {k}: {v}"), is there a preference in style?

[jlduran]

In D38053#867961, @melifaro wrote:

4. In the meantime the kernel delayed object reclamation model issue is fixed, maybe adapt 80fc25025ffcb0d369fc0b6d4d272ad6fd3f53c3 ?

Committed in 20ea7f26e413 .

Amazing. Thank you!

[jlduran]

In D38053#867921, @melifaro wrote:

3. An atf_get_srcdir-equivalent to eventually read files relative to the source directory, however I think the idea is to disassociate from ATF later on.

This one slipped through the cracks, thanks for reminding! Will add in a day or two.

I thought of passing it via wrapper initially, but ended up with a bit more natural way - getting it from pytest itself.
I committed an example here.

@melifaro, I left a comment on GitHub, I guess you're not receiving those:
https://github.com/freebsd/freebsd-src/commit/864ea9abfb98493a157dc17be17c428080843bdd#commitcomment-98160625
I had to make a few changes in order to make it PASS.

[ngie]

In D38053#867963, @jlduran_gmail.com wrote:

In D38053#867921, @melifaro wrote:

3. An atf_get_srcdir-equivalent to eventually read files relative to the source directory, however I think the idea is to disassociate from ATF later on.

This one slipped through the cracks, thanks for reminding! Will add in a day or two.

I thought of passing it via wrapper initially, but ended up with a bit more natural way - getting it from pytest itself.
I committed an example here.

Ha, very nice! I tend to prefer the f-string format (i.e., print(f" {k}: {v}"), is there a preference in style?

f-strings are fine given the FreeBSD v cpython support matrix, as long as you use 3.8+ only extensions to f-strings.

sbin/ping/tests/test_ping.py

111

I recommend using type hints instead of encoding the types for parameters and return values in functions/method docstrings: it eliminates useless metadata and allows other tools like mypy or pylint to analyze the type hints and help evaluate correctness.

270–275

A lot of output metadata is being baked into the tests. I would consider abbreviating the responses somehow, or maybe make the parameters into a higher-level global dictionary, so some of the indentation can be reduced.
Also, some of these test cases rely on IPv4 and/or IPv6 support being enabled in the kernel: I would consider adding decorators and/or fixtures to check for this address family support before running a series of tests which will fail.

410

Coming up with human-readable descriptions of this would probably be better, e.g., "audible_bell_three_count_ipv4".

[jlduran]

@ngie Thank you for your suggestions, I'll try to implement them!
I also plan to update this file, hopefully this week, I'll remove/skip the tests for D37930 (as it hasn't landed yet), and start submitting the proposed fixes for the utility itself. I don't think all of them will be able to make it to the upcoming release, so there isn't really a time constraint, but I would like to start receiving feedback on that as well. Admittedly, having nice tests usually facilitates it!

sbin/ping/tests/test_ping.py

111

Good idea! I'll try that.

270–275

A lot of output metadata is being baked into the tests. I would consider abbreviating the responses somehow, or maybe make the parameters into a higher-level global dictionary, so some of the indentation can be reduced.

Unfortunately we'll need almost all that info. This file will harbor a lot more tests and every single thing in the response needs to be scrutinized, the abstraction you are proposing would imply nearly re-implementing ping's printf routines here? I would prefer if we can avoid that! There are may more tests coming, e.g., we have opted to not remove the negative sign of the time, as there was a PR (now closed #192417), that we can artificially trigger, and we need the sign to identify when the statistics have been tampered, and void them, for instance.

Also, some of these test cases rely on IPv4 and/or IPv6 support being enabled in the kernel: I would consider adding decorators and/or fixtures to check for this address family support before running a series of tests which will fail.

The IPv4/IPv6 support should automagically be handled by ATF-Python, if not fully at the moment, eventually in the future it will. The plan is to polish this testing framework as we go along. Ideally, let the framework work for us, so we don't need to worry about implementing those details on every test. This was also one of the reasons why we decided to try ATF-Python; with ATF-sh a lot of this repetition is needed, bloating even more the tests.

410

Originally I had these names programmatically created, because a lot more of them would be added. We'll be testing nearly every command option. But, yes, whenever possible, I'll try adding a descriptive, human name to the test.
With ATF-sh, this was the "descr", I wonder if we could adapt something similar here?

[ngie]

In D38053#872961, @ngie wrote:

In D38053#867963, @jlduran_gmail.com wrote:

In D38053#867921, @melifaro wrote:

3. An atf_get_srcdir-equivalent to eventually read files relative to the source directory, however I think the idea is to disassociate from ATF later on.

This one slipped through the cracks, thanks for reminding! Will add in a day or two.

I thought of passing it via wrapper initially, but ended up with a bit more natural way - getting it from pytest itself.
I committed an example here.

Ha, very nice! I tend to prefer the f-string format (i.e., print(f" {k}: {v}"), is there a preference in style?

f-strings are fine given the FreeBSD v cpython support matrix, as long as you use 3.8+ only extensions to f-strings.

I meant <3.8 compatible f-string extensions — there are some items that have been introduced in 3.9/3.10 that won’t work with prior versions of python.

[jlduran]

Updates:

• Use ToolsHelper.set_sysctl
• Use type hints
• Skip failing tests
• Remove tests of features not yet implemented

[jlduran]

• Do not shadow opts var
• Disable net.inet.ip.process_options for RR options as well (this was discovered by randomizing the tests, as this sysctl was set by the previous tests)

[jlduran]

• Add a missing extra white space

The output was originally taken from a branch that has code that trims these extra spaces. Let's leave those cosmetic changes for last.

[melifaro]

This revision is up for a while. What do you folks think of committing some or all of these tests that don't fail or current ping?
Nothing stops us from committing the other tests (or fixing some of the current tests) along with the ping changes or separately.

[jlduran]

Yes, please!
In the meantime, I'll keep updating this "BASE" revision.
Changes:

• Add the ability doctor IHL fields
• Add the ability to remove inner packet load

[melifaro]

In D38053#876436, @jlduran_gmail.com wrote:

Yes, please!
In the meantime, I'll keep updating this "BASE" revision.
Changes:

• Add the ability doctor IHL fields
• Add the ability to remove inner packet load

I'm sorry for the delay, was focusing on fixing staff for the upcoming 13.2 release.