NOTE: I will probably abandon this in favor of adding ATF tests directly in tcptestsuite.
Details
Details
Diff Detail
Diff Detail
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 24817 Build 23569: arc lint + arc unit
Event Timeline
Comment Actions
The basic idea behind this review is fundamentally broken. We shouldn't be adding test cases to the base system that wrap a port which is itself just a bunch of test cases. Instead, the correct approach would be to add ATF test cases directly to the port, to be installed into /usr/local/tests/tcptestsuite. See devel/atf, devel/kyua, or devel/lutok for examples of ports that do that.
| tests/sys/tcptestsuite/Makefile | ||
|---|---|---|
| 8 | What's the point of this file? | |
| tests/sys/tcptestsuite/tcptestsuite_test.sh | ||
| 1 | You can leave out the shebang. The build process adds it. | |
| 36 | This is way too complicated. Instead, just put an atf_require_prog tcptestsuite and atf_require_prog packetdrill in each test case's body. Or better yet, just add TESTS_METADATA+= required_programs="packetdrill tcptestsuite" to the Makefile. | |
| 50 | find . works better than find *. Same output, but no shell globbing. | |
| 62 | Why the sleep? | |
Comment Actions
I wasn't aware of that integration with ports. I agree, that's a better way. I'll take that approach. Thanks for setting me on the right track.
| tests/sys/tcptestsuite/Makefile | ||
|---|---|---|
| 8 | None yet. :) Known-bad tests can be listed here. Or it might go away. | |
| tests/sys/tcptestsuite/tcptestsuite_test.sh | ||
| 1 | I added it mostly as documentation for humans, and possibly for editors as a syntax highlighting hint. | |
| 36 | Note that tcptestsuite is just a directory, not a program. I wasn't sure how to use this metadata to specify a required directory (and I wanted to get this out for review sooner, which I'm glad I did, due to your feedback). | |
| 50 | The output is different: it begins with ./. It was important to avoid this in an earlier draft. Now it would be fine. | |
| 62 | Because the main script(s) in the tcp-testsuite repo sleep between each test. This is a WIP, and the review is mostly a place to discuss it with @tuexen. | |
Comment Actions
I wasn't aware of that integration with ports. I agree, that's a better way. I'll take that approach. Thanks for setting me on the right track.
If you add the tests directly in GH, feel free to request a review from me.
| tests/sys/tcptestsuite/Makefile | ||
|---|---|---|
| 8 | I suggest that you remove it, and instead use atf_skip in the test case's body. You should probably also open a PR (or github issue) for each skipped test and reference it in atf_skip's message. That way future developers will know when it's safe to reenable the test. | |
Comment Actions
If you add the tests directly in GH, feel free to request a review from me.
Here is the commit:
https://github.com/vangyzen/tcp-testsuite/commit/c0066c22311acd91df42bc8c525e2886bef6eac9
Here is the pull request:
https://github.com/freebsd-net/tcp-testsuite/pull/4
I couldn't add you as a reviewer on the PR, presumably because neither of us is a member of the freebsd-net project. Maybe @tuexen will think we're cool enough to invite us. :)