Add test cases for cat(1)

[shivansh]

Summary

• Separate the test cases for '-e', '-b' and '-s' options. Add relevant input and output files.
• Update d_align.out to match the expected output of '-b' option.
• Test that '-vt' option displays non-printing characters, namely control characters, delete character and meta-characters.

Test Plan

• Run make install from src/bin/cat/tests.
• Run kyua test from /usr/tests/bin/cat. All 5 test cases should succeed.

[brooks]

Why have your copied this files that are already in the source tree?

[asomers]

bin/cat/tests/cat_test

1 ↗

(On Diff #29122)

ATF-sh files should omit the shebang line. It will be added by make

2 ↗

(On Diff #29122)

You should use a fresh license block instead of copying NetBSD's, because this is a new file. You can update the copyright to 2017 and list yourself as the author. Use this template:
https://www.freebsd.org/doc/en/articles/committers-guide/pref-license.html

39 ↗

(On Diff #29122)

Eliminate the blank line here on and line 62

41 ↗

(On Diff #29122)

You should usually avoid atf_check's -x option. You can simply remove it and remove the quotes and all should be fine.

61 ↗

(On Diff #29122)

Please keep body sections adjacent to their corresponding head sections.

bin/cat/tests/d_e_output.in

2 ↗

(On Diff #29122)

This file looks identical to contrib/netbsd-tests/bin/cat/d_se_output.in . Despite the fact that you're using it a little differently, there's no need to duplicate it. Just reference the older file.

[shivansh]

Address changes mentioned by @asomers

• Update license.
• Omit the shebang line from ATF-sh file cat_test.sh.
• Eliminate the usage of atf_check's -x option.
• Eliminate blank lines.
• Remove duplicate test input file d_e_output.in.

[ngie]

I understand what you are doing, but not why you are doing it. Can you please explain (in more detail), why you think this change is required?

If your goal is to add more coverage, we can add more coverage to the existing test scripts -- otherwise, I disagree with your approach.

[ngie]

In D11020#228268, @ngie wrote:

I understand what you are doing, but not why you are doing it. Can you please explain (in more detail), why you think this change is required?

If your goal is to add more coverage, we can add more coverage to the existing test scripts -- otherwise, I disagree with your approach.

Background: I had to clean up a lot of "choices" made by contractors hired by EMC that did similar things, for no benefit. I'd rather not repeat history.

[shivansh]

I was advised by @asomers not to make any updates in contrib/netbsd-tests/bin/cat and that they should be made in bin/cat/tests.
If it is advisable to make updates in the former, I'll do so accordingly.

[ngie]

In D11020#228310, @shivansh wrote:

I was advised by @asomers not to make any updates in contrib/netbsd-tests/bin/cat and that they should be made in bin/cat/tests.
If it is advisable to make updates in the former, I'll do so accordingly.

There might be some confusion here:

1. It's ok adding tests to contrib/netbsd-tests/bin/cat, if they are generic enough that they can be run on NetBSD. If you're adding new tests to anywhere under contrib/netbsd-tests/... I ask that sufficient annotation be added around changes made (/* Begin FreeBSD */ -> /* End FreeBSD */ for C, // Begin FreeBSD -> // End FreeBSD for C++, or # Begin FreeBSD -> # End FreeBSD for shell tests), and I be added to reviews, so I can push for their upstreaming (I'm basically the maintainer for contrib/netbsd-tests/... ).
2. For any FreeBSD-specific tests, they should go into separate test scripts, e.g., bin/cat/tests/cat_test2.sh; I've been considering renaming <foo>_test2.sh to freebsd_<foo>_test.sh in the future, if they're for FreeBSD-specific feature tests, but I haven't gotten there yet (I need to setup a proposal, formalize the naming scheme, and execute on that).

I don't see any value to copy-pasting the NetBSD tests and splitting them up... please don't do that and instead try to work within the guidelines I noted above.

Thanks!

[shivansh]

Update the introduced changes pertaining to guidelines stated by @ngie

[asomers]

Ok with me if it's ok with @ngie

[ngie]

• Why change existing tests?
• Could you please add Begin FreeBSD/End FreeBSD around changes, so it's easier for me to pick out what we have and haven't modified, e.g.,

72         # Begin FreeBSD
73         grep_type
74         if [ $? -eq $GREP_TYPE_GNU ]; then
75                 atf_expect_fail "this test doesn't pass with gnu grep from ports"
76         fi
77         # End FreeBSD

?

[shivansh]

Address comment by @ngie -

• I separated the test-cases for options which are responsible for different functionality.
• I earlier added (Begin FreeBSD...End FreeBSD) around only new test-case definitions, but now extended them for all the modifications done (including the Makefile).

[ngie]

I'm not sure why se_output is being deleted/redone to be honest -- it was added specifically for a NetBSD PR; what you're doing is adding potentially new coverage with the -e and the -s flags, which may or may not be covered in the -se case :).

bin/cat/tests/Makefile

9 ↗

(On Diff #29273)

Begin/End FreeBSD isn't needed for the Makefiles -- just the stuff that lives under contrib/netbsd-tests ;)..

[shivansh]

Revert changes done to the test-case se_output

[ngie]

contrib/netbsd-tests/bin/cat/t_cat.sh

31–55 ↗

(On Diff #29275)

I still don't understand why you're making these changes. Please revert them.

63–67 ↗

(On Diff #29275)

Unnecessary upstream diff.

[shivansh]

• Revert unrequired changes

I read that atf-check's '-x' option should be avoided to prevent shell quoting, so these got overlooked. Sorry.

[ngie]

LGTM -- thank you very much for going through the process of getting this polished up for commit.

I'll submit the -x changes upstream -- it's just one extra thing I'd prefer to not have in the FreeBSD repository to have to upstream.