sort: Make NetBSD sort tests compatible with sort.
ClosedPublic
Actions

Authored by cyril_freebsdfoundation.org on May 11 2021, 6:42 PM.

Details

Reviewers

emaste
markj
ngie

Commits

rGf2f241349486: sort: Make NetBSD sort tests compatible with our sort
rGf9e565abca9b: sort: Make NetBSD sort tests compatible with our sort

Summary

This diff primarily adds/removes flags to make the tests compatible with sort. Two tests are removed. One test is changed to expect fail due to a bug.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

cyril_freebsdfoundation.org created this revision.May 11 2021, 6:42 PM

Herald added a reviewer: ngie. · View Herald TranscriptMay 11 2021, 6:42 PM

cyril_freebsdfoundation.org requested review of this revision.May 11 2021, 6:42 PM

cyril_freebsdfoundation.org removed a reviewer: ngie.

Herald added a reviewer: ngie. · View Herald TranscriptMay 11 2021, 6:43 PM

markj added inline comments.May 11 2021, 6:50 PM

contrib/netbsd-tests/usr.bin/sort/t_sort.sh
960	Rather than commenting out tests, it is better to add `atf_skip` calls at the beginning of each test. This reduces the number of local modifications to this file, which makes merging easier when changes are imported from NetBSD.

These are a lot of changes to an upstream (NetBSD) provided file without good annotations (please look for "Begin FreeBSD" elsewhere).

As @markj noted, making broad changes like this will definitely make my job a lot harder when I update this project. Using atf_expect_fail is an alternative to atf_skip, as it would trigger a legitimate failure if the code doesn't fail as expected.

Please put FreeBSD-specific assertions in separate test cases as well if they are FreeBSD specific.

I reduced the amount of modified lines and added comments throughout describing changes. @markj recommended that I avoid using Begin FreeBSD and End FreeBSD annotations throughout the file in order to reduce clutter.

lwhsu added a subscriber: lwhsu.May 12 2021, 1:22 AM

Thanks, this looks good to me aside from the comments.

contrib/netbsd-tests/usr.bin/sort/t_sort.sh
330–332	Please add a "On FreeBSD, ..." qualifier so that it's clear that this comment isn't from upstream.
913	This actually looks the same as PR 255798?

cyril_freebsdfoundation.org added inline comments.May 12 2021, 4:56 PM

contrib/netbsd-tests/usr.bin/sort/t_sort.sh
913	Flags for sort can come after filenames; commands like `sort in -r` don't treat `-r` as a filename. Since `+0` is a valid flag, then `sort in +0` shouldn't take `+0` as a filename in this case.

markj accepted this revision.May 12 2021, 8:26 PM

markj added inline comments.

contrib/netbsd-tests/usr.bin/sort/t_sort.sh
913	Oof, that's weird. Looking at NetBSD's sort, they appear to stop mangling `+` options after encountering an option that doesn't start with `-` or `+`. We could perhaps implement that in D30234 as well. But our sort already behaves the same as GNU sort in this case, so it's probably better to keep things as they are.