Page MenuHomeFreeBSD

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.
Referenced Files
Unknown Object (File)
Fri, Apr 19, 11:03 AM
Unknown Object (File)
Feb 21 2024, 4:33 AM
Unknown Object (File)
Feb 5 2024, 9:37 PM
Unknown Object (File)
Jan 24 2024, 7:19 AM
Unknown Object (File)
Dec 22 2023, 11:16 PM
Unknown Object (File)
Dec 18 2023, 3:02 PM
Unknown Object (File)
Dec 14 2023, 10:54 PM
Unknown Object (File)
Dec 11 2023, 12:37 AM



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.

Test Plan

Fail the additional kyua tests while still passing the kyua tests we previously passed

Diff Detail

rS FreeBSD src repository - subversion
Lint Not Applicable
Tests Not Applicable

Event Timeline

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?

Add a test for passing zero length (invalid) expressions to egrep

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.


185 ↗(On Diff #26587)

Why is this being added to this test?

301–305 ↗(On Diff #26587)

Why are you relying specifically on jemalloc(3) features to validate grep? It seems prone to breakage in the future.


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, 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. =)

185 ↗(On Diff #26587)

It's a thing that we've broken in the past, and I think it's worthwhile to make sure we don't break it again in the future as those bits of bsdgrep(1) can still use some more attention. See PR 194823 for recent breakage.

301–305 ↗(On Diff #26587)

Generally just to make sure that we don't end up with a case like PR 175314 again, although I guess really the same scenario can happen just about anywhere, making this half bogus. I'll reduce this to the basics without jemalloc(3) bits.

185 ↗(On Diff #26587)

Sorry, I meant to add that it seemed appropriate given that it is also a test of egrep not doing bad things.

Updated to remove reliance on jemalloc(3) and include \# Begin FreeBSD && \# End FreeBSD markers as per @ngie

kevans marked 2 inline comments as done.

Match color tests up with gnugrep(1) expectation that \33[K is only added to end of colored output

This revision is now accepted and ready to land.Mar 23 2017, 7:30 PM
kevans edited edge metadata.
  • Add _head()/descriptions for consistency sake, back egrep test out into its own test

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

This revision now requires review to proceed.Mar 24 2017, 4:38 PM
This revision is now accepted and ready to land.Mar 24 2017, 5:40 PM
kevans edited edge metadata.
  • Revise coloring expectation based on discussion in response to r316477
This revision now requires review to proceed.Apr 4 2017, 2:02 PM

It would be nice to reference PRs where the test consists of an example from the PR.

  • Reference PRs used for specific tests
This revision is now accepted and ready to land.Apr 4 2017, 4:55 PM
ngie added inline comments.
233 ↗(On Diff #27032)

*Check (subject-verb agreement as the direct object is "behavior", not "matches")

270 ↗(On Diff #27032)

Check --color support

291 ↗(On Diff #27032)

in empty file, specified by -f

303 ↗(On Diff #27032)

Wordy description.

314 ↗(On Diff #27032)

"invalidly" sounds weird. I think you mean "invalid"?

11–15 ↗(On Diff #27032)

The color tests might not be upstream able as-is, because NetBSD's running gnu grep 2.5.1a-nb1. I'll have to double-check later.

kevans edited edge metadata.
  • Fix wording, recommendations by @ngie
This revision now requires review to proceed.Apr 4 2017, 6:14 PM
kevans added inline comments.
303 ↗(On Diff #27032)

How does the new wording look to you? I modified it to better match what it essentially means from the perspective of a grep user, rather than one working on grep.

11–15 ↗(On Diff #27032)

Happy to amend them or move them around as needed. =) Should this be done before making it in, or is it OK to sort this out at a later time?

ngie added inline comments.
11–15 ↗(On Diff #27032)

Let's just go with what you have as-is. I'm fine working out the issues as long as I have a heads up/bandwidth to do so.

This revision is now accepted and ready to land.Apr 4 2017, 6:26 PM

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]
In D10112#212507, @ngie wrote:

It looks like the tests are pending fixes in the other reviews...

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.

Ping @emaste -- any further comments/action required on this one? =)

This revision was automatically updated to reflect the committed changes.