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)
Details
Details
- Reviewers
asomers - Commits
- rS351393: ping: add a basic functional test
Diff Detail
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
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.
Comment Actions
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?
Comment Actions
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.
Comment Actions
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?
Comment Actions
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.
Comment Actions
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:
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. |
| sbin/ping/tests/ping_test.sh | ||
|---|---|---|
| 24 ↗ | (On Diff #60988) | I meant adding -u as an option to diff. |
| 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. |
Comment Actions
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.
Comment Actions
Rebase.
Send only one packet.
Add -t 1 option to reduce time of waiting for the reply.