- 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.
Details
- Reviewers
asomers brooks ngie - Commits
- rS319634: Add additional testcases for cat(1)
- Run make install from src/bin/cat/tests.
- Run kyua test from /usr/tests/bin/cat. All 5 test cases should succeed.
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
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: |
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. |
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.
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.
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:
- 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/... ).
- 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!
- 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
?
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).
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 ;).. |
- Revert unrequired changes
I read that atf-check's '-x' option should be avoided to prevent shell quoting, so these got overlooked. Sorry.
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.