Page MenuHomePhabricator

[llvm-strings] Improve testing of llvm-strings
ClosedPublic

Authored by jhenderson on Aug 9 2019, 8:20 AM.

Details

Summary

This patch attempts to tidy up the llvm-strings testing by:

  1. Adding comments to every test.
  2. Getting rid of canned input files, and have the tests generate them on the fly (this makes the tests self-contained).
  3. Adding missing test coverage.
  4. Renaming some tests that weren't clear as to their purpose.
  5. Adding extra checking of various cases, formatting etc.
  6. Removing a test that didn't seem to have any useful purpose for testing llvm-strings.

Diff Detail

Repository
rL LLVM

Event Timeline

jhenderson created this revision.Aug 9 2019, 8:20 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 9 2019, 8:20 AM
rupprecht accepted this revision.Aug 9 2019, 8:54 AM

Just one test that needs fixing, LGTM otherwise:

test/tools/llvm-strings/version.test
4 ↗(On Diff #214381)

This is actually testing the default cmake config; see this comment from llvm/test/tools/llvm-readobj/basic.test:

# Test version switch.
RUN: llvm-readobj --version | FileCheck %s --check-prefix=VERSION
RUN: llvm-readelf --version | FileCheck %s --check-prefix=VERSION
# In default configuration we could match "LLVM version", but the "LLVM" part
# can be changed with PACKAGE_NAME in CMake, so we match only version.
VERSION: version
This revision is now accepted and ready to land.Aug 9 2019, 8:54 AM
MaskRay accepted this revision.Aug 9 2019, 9:56 PM
grimar added inline comments.Aug 10 2019, 5:42 AM
test/tools/llvm-strings/all-sections.test
1 ↗(On Diff #214381)

Should we use ## in this tests too? I would do this, since it is probably
not bad to have a single style across the all LLVM tests.

test/tools/llvm-strings/eof-no-string.test
2 ↗(On Diff #214381)

I think it just does not print all strings that are shorter than 4 bytes?
I.e. not only the last line.

test/tools/llvm-strings/eof.test
5 ↗(On Diff #214381)

What about combining this and eof-no-string.test test into one?

test/tools/llvm-strings/file-filename.test
10 ↗(On Diff #214381)

My emotion for this test at first was: "what about multiple inputs?". Then I found
multiple-inputs.test test below. I would just inline it here.

test/tools/llvm-strings/help.test
12 ↗(On Diff #214381)

Note: I am not sure why "Options" is in upper case when "Generic", but lower case when "General",
but my concern is that it makes the test case to be potentially brittle probably:

If you have --implicit-check-not="Generic Options:", then if somebody deside to change "Option"->"option",
then you'll get a broken test case. Not sure if we should think about this scenario in that way though.

test/tools/llvm-strings/length.test
13 ↗(On Diff #214381)

What if -n 6 or -n -1 or -n foo?

test/tools/llvm-strings/version.test
1 ↗(On Diff #214381)

I.d probably just put this into help.test. What do you think?

jhenderson marked 10 inline comments as done.Aug 12 2019, 3:07 AM
jhenderson added inline comments.
test/tools/llvm-strings/all-sections.test
1 ↗(On Diff #214381)

I didn't bother here because this isn't used as an input to anything, but I can do so if you're bothered. Should I add '#' to the RUN and CHECK lines too? I'd prefer not to, mostly because that will unnecessarily make git blame less useful.

test/tools/llvm-strings/eof-no-string.test
1 ↗(On Diff #214381)

I'm not sure why my patch isn't showing this, but this has been renamed from "terminator-neg.test".

2 ↗(On Diff #214381)

Right, but this is specifically about the last line case (there are other cases for the general behaviour). A basic implementation would look something like:

const char *S = nullptr;
for (const char *P = Str.begin(); P != Str.end(); ++P) {
  if (!isPrint(P)) {
    if (P - S >= 4)
      printStr(S, P);
    S = nullptr;
  }
}

This wouldn't print the trailing characters at all. A naive fix would add the following after the loop:

if (S != nullptr)
  printStr(S, Str.end());

When the check should actually be if (S != nullptr && Str.end() - S >= 4). So this test case is checking that second clause in the final if.

Note that this and the original eof.test were written to fix a bug where that final if was missing.

test/tools/llvm-strings/eof.test
1 ↗(On Diff #214381)

I'm not sure why my patch isn't showing this, but this has been renamed from "terminator.test".

5 ↗(On Diff #214381)

I could do that, but I kept them separate because that's how they were previously. If you'd like me to fold them together, I can do so.

test/tools/llvm-strings/file-filename.test
10 ↗(On Diff #214381)

Okay.

test/tools/llvm-strings/help.test
12 ↗(On Diff #214381)

I agree that the inconsistent case is bad. I'm not sure of a good alternative (apart from making it a regex pattern of [Oo]). I'll give it a bit more thought.

test/tools/llvm-strings/length.test
13 ↗(On Diff #214381)

-n -1 is actually broken. It looks like the command-line parser converts it to an unsigned, resulting in it being the equivalent of UINT_MAX. As GNU strings allows -1 as an alias for -n 1, we probably don't want to try to address this immediately, and fix that issue first, then see if the problem is still possible afterwards.

I'll add -n foo and -n 6 cases.

test/tools/llvm-strings/version.test
1 ↗(On Diff #214381)

I prefer to keep each switch to a separate test, unless they're intricately linked (in this case --help and --version are not really linked).

4 ↗(On Diff #214381)

Thanks! I'll fix that.

jhenderson marked an inline comment as done.Aug 12 2019, 3:18 AM
jhenderson added inline comments.
test/tools/llvm-strings/file-filename.test
10 ↗(On Diff #214381)

Actually, multiple-inputs.test isn't just about the --print-file-name switch. It's more generally about how llvm-strings works with multiple inputs, i.e. it reads each of them in turn, and produces correct output with --print-filename and --radix when switching between files, so I think it belongs as a separate test.

Possibly I could be persuaded that the --print-file-name and --radix checks for multiple inputs belong in their respective tests, but I think multiple inputs still need testing separately.

grimar added inline comments.Aug 12 2019, 3:22 AM
test/tools/llvm-strings/all-sections.test
1 ↗(On Diff #214381)

I'd use '##' fo comments, I agree it is probably no need to use them for RUN/CHECK lines.

test/tools/llvm-strings/eof.test
5 ↗(On Diff #214381)

Phab did not show the renaming, so I assumed this is the new file.
Since it is not, it is OK to leave it as is in this patch I think.

(I would fold them in a followup though).

jhenderson marked 8 inline comments as done.

Addressed review comments:

  • Made comments use double hash.
  • Made some tests less brittle.
  • Added extra test cases as suggested.
test/tools/llvm-strings/help.test
12 ↗(On Diff #214381)

I've tried making it less likely to get missed, by putting the -NOT checks out-of-line, so that they're next to the positive checks. If the case is changed, hopefully the person updating the test will change both positive and negative cases.

test/tools/llvm-strings/length.test
13 ↗(On Diff #214381)

-n -1 is actually broken. It looks like the command-line parser converts it to an unsigned

Turns out it's an int in the parser, so the parser accepts it, but it is then static_cast to a size_t! Anyway, still broken, and I've filed a bug for negative arguments.

grimar accepted this revision.Aug 12 2019, 3:46 AM

LGTM.

jhenderson marked an inline comment as done.Aug 12 2019, 4:31 AM
jhenderson added inline comments.
test/tools/llvm-strings/stdin.test
11 ↗(On Diff #214596)

On my Linux box, "echo -e" doesn't work without quoting the string to be echoed, so I did that throughout this test.

This revision was automatically updated to reflect the committed changes.