Page MenuHomeFreeBSD

ping: add a basic test of ping's functionality
ClosedPublic

Authored by sucanjan_gmail.com on Aug 16 2019, 7:56 AM.

Details

Summary

Add a basic test of ping's functionality

Submitted by: Ján Sučan <sucanjan@gmail.com>
Sponsored by: Google, inc. (Google Summer of Code 2019)

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Should I put any copyright information to tests/ping_test.sh?

asomers requested changes to this revision.Aug 17 2019, 2:35 PM

This test will fail if IPv4 is not configured. It needs some kind of test for that. Also, 127.0.0.1 is merely the conventional address for localhost. It's possible to configure localhost's address to 127.0.0.2, for example.

This revision now requires changes to proceed.Aug 17 2019, 2:35 PM

It seems that that are far more variable parts in the output than I thought. I think the exact ttl value and ordering of responses cannot be relied on either. Is it OK having a test like this with all of that numeric information filetered out?

Use a single tab width in the ATF test script.

Skip the test case if IPv4 is not configured.

Remove IP addresses from ping's output.

It seems that that are far more variable parts in the output than I thought. I think the exact ttl value and ordering of responses cannot be relied on either. Is it OK having a test like this with all of that numeric information filetered out?

That's what I was originally worried about. Ping is hard to test because it's so nondeterministic. If you filter out everything variable, will the remainder be a sufficiently useful test?

Well, I'm not sure. It's not completely useless. It helped me to discover few bugs during development. Maybe we could keep it just for now when there is no better testing employed. Filtering those information out itself could be considered a check because if a string doesn't match the regular expression, it's not filtered out and it's detected. Please, decide. If you close this review (and the one for ping6), I will understand and I will continue without the ATF sh tests.

This test is certainly better than nothing. I wish we had something better, but I can't come up with a superior test myself. I'll accept this in lieu of something perfect.

sbin/ping/tests/ping_c3.out
2 ↗(On Diff #60988)

Are you planning to filter out the ttl value?

sbin/ping/tests/ping_test.sh
24 ↗(On Diff #60988)

What does this output look like on a failure? Would it be more readable if you added -u?

sbin/ping/tests/ping_c3.out
2 ↗(On Diff #60988)

Yes. The value is dependent on net.inet.ip.ttl sysctl. At first I was thinking about using value of sysctl -n net.inet.ip.ttl in the regular expression but it probably opens possibilities for errors (e.g., privileges for executing sysctl command).

sbin/ping/tests/ping_test.sh
24 ↗(On Diff #60988)

The output looks like:

ping_test:ping_c3 -> failed: atf-check failed; see the output of the test for details`

atf-sh doesn't recognize -u option but I think the question was about specifying a reason. I'm afraid that this won't make the output more readable because there are too much information included in it to be described by a single message.

sbin/ping/tests/ping_test.sh
24 ↗(On Diff #60988)

I meant atf_check instead of atf-sh.

asomers added inline comments.Aug 20 2019, 3:25 PM
sbin/ping/tests/ping_test.sh
24 ↗(On Diff #60988)

I meant adding -u as an option to diff.

asomers added inline comments.Aug 20 2019, 3:31 PM
sbin/ping/tests/ping_c3.out
2 ↗(On Diff #60988)

Instead of relying on the default or filtering it out, why not specify it on the command line? In addition to being more robust against environmental changes, that will provide better test coverage. You could also specify -s, and -h for the ping6 test.

sbin/ping/tests/ping_c3.out
2 ↗(On Diff #60988)

I tried to set TTL but it didn't affect the output. I realized that it's only for outgoing packets. Incoming packets have TTL set by the target system.

I will use -s option.

Filter out TTL value.
Specify packet size on the command line.
Add -u option to diff. This makes the result file more readable in case of an error.

This needs a rebase now.

Rebase.
Send only one packet.
Add -t 1 option to reduce time of waiting for the reply.

asomers accepted this revision.Aug 22 2019, 2:13 PM
This revision is now accepted and ready to land.Aug 22 2019, 2:13 PM
asomers requested changes to this revision.Aug 22 2019, 2:18 PM

Please add a copyright header with a $FreeBSD$ tag to the top of ping_test.sh

This revision now requires changes to proceed.Aug 22 2019, 2:18 PM

Add a copyright header with a $FreeBSD$ tag to ping_test.sh.

asomers accepted this revision.Aug 22 2019, 2:58 PM
This revision is now accepted and ready to land.Aug 22 2019, 2:58 PM
This revision was automatically updated to reflect the committed changes.