Differential D10112
bsdgrep(1): Create additional tests to expand coverage into areas that we're currently lacking Authored by kevans on Mar 23 2017, 2:36 AM. Tags None Referenced Files
Subscribers None
Details Create additional tests to cover regressions that were discovered by PRs linked to D10098, D10102, and D10104. It is worth noting that neither bsdgrep(1) nor gnugrep(1) in the base system currently pass all of these tests, and gnugrep(1) not quite being up to snuff was also noted in at least one of the PRs. Additionally, I have no idea how this should be reconciled with upstream. I don't know what their current status is, but I assume they're in the same boat as us as far as grep(1) goes. Fail the additional kyua tests while still passing the kyua tests we previously passed
Diff Detail
Event TimelineComment Actions The color test fails with gnugrep(1) because of slight differences in coloring approach -- it looks to be specifically how they terminate the color, judging by output. Should I address this by doing nothing, stripping colors, or altering bsdgrep(1) to terminate colors in the same fashion? Comment Actions Hi Kyle, A few general comments: - With changes to NetBSD tests, I ask that the tests be annotated. This makes it a lot easier for me to find modifications made to the NetBSD tests when upstreaming them back to NetBSD. In particular: -- C/C++: Add `/* Begin FreeBSD */` at the start of the modified block and add `/* End FreeBSD */`. -- sh: Add `# Begin FreeBSD` at the start of the modified block and add `# End FreeBSD */`. - Please don't modify the style of the code, if at all possible, because stylistic changes become gratuitous. - Ask the question: do these tests warrant upstreaming? If not, they should go in a separate test script, as they're FreeBSD-specific. Thanks!
Comment Actions Hello! Thanks for that! A couple of questions in response: I think the tests I've added are a good candidate for inclusion in NetBSD, since they're not really FreeBSD or bsdgrep(1) specific. gnugrep(1) passes all of the tests I've added except it has some legitimately and apparently known bugs with the -o flag, so oflag_zerolen fails. Is it still alright to consider these upstream-able? These are sort of written with the eventual goal of replacing gnugrep(1) with bsdgrep(1) in mind and coming more in line with newer GNU grep behaviors, but I can't really say what NetBSD would do in this regard. I will go ahead and fix the --color output of bsdgrep(1) to match gnugrep(1), though, just to not cause completely needless test failures. I have attempted to maintain the style of the rest of t_grep.sh, but had internal conflicts when it came to _head() functions. I've read documentation somewhere that ATF tests are preferred to have descriptive enough names to not require a _head() function, but every other test in here does indeed have a _head() function. Should I introduce them to be consistent, or go with other advice to make decent test names? Thank you for your input. =)
Comment Actions Updated to remove reliance on jemalloc(3) and include \# Begin FreeBSD && \# End FreeBSD markers as per @ngie Comment Actions Match color tests up with gnugrep(1) expectation that \33[K is only added to end of colored output Comment Actions
Based on feedback by @ngie. Leaving these in the general test set, rather than separating them out, given that none of them are really FreeBSD specific
Comment Actions It looks like the tests are pending fixes in the other reviews... grep_test:basic -> passed [0.024s] grep_test:begin_end -> passed [0.025s] grep_test:binary -> passed [0.022s] grep_test:color -> passed [0.037s] grep_test:context -> failed: atf-check failed; see the output of the test for details [0.032s] grep_test:context2 -> failed: atf-check failed; see the output of the test for details [0.021s] grep_test:egrep -> passed [0.020s] grep_test:egrep_empty_invalid -> passed [0.018s] grep_test:escmap -> passed [0.025s] grep_test:f_file_empty -> failed: atf-check failed; see the output of the test for details [0.018s] grep_test:file_exp -> passed [0.021s] grep_test:ignore_case -> passed [0.018s] grep_test:invert -> passed [0.021s] grep_test:negative -> passed [0.019s] grep_test:nonexistent -> passed [0.018s] grep_test:oflag_zerolen -> passed [0.050s] grep_test:recurse -> passed [0.023s] grep_test:recurse_symlink -> passed [0.021s] grep_test:whole_line -> passed [0.019s] grep_test:word_regexps -> passed [0.020s] grep_test:xflag -> passed [0.021s] grep_test:zgrep -> passed [0.022s] Comment Actions Indeed, @cem requested that the tests go in either around the same time or before the fixes for the issues tested. Presumably to make sure that the coverage really does get included and it doesn't go by the way-side later. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||