[PATCH] D66015: [llvm-strings] Improve testing of llvm-strings

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: test/tools/llvm-strings/all-sections.test:1
-RUN: llvm-strings -a %S/Inputs/abcd | FileCheck %s
-RUN: llvm-strings --all %S/Inputs/abcd | FileCheck %s
+# Show that the -a/--all switch is accepted. GNU strings checks specific object
+# file sections unless --all is specified. llvm-strings just looks at the entire
----------------
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.


================
Comment at: test/tools/llvm-strings/eof-no-string.test:2
+# Show that llvm-strings does not print the last string in the input if it is
+# too short and no unprintable character follows it.
+
----------------
I think it just does not print all strings that are shorter than 4 bytes?
I.e. not only the last line.


================
Comment at: test/tools/llvm-strings/eof.test:5
 RUN: echo -n abcdefg | llvm-strings - | FileCheck %s
 CHECK: abcdefg
----------------
What about combining this and `eof-no-string.test` test into one?


================
Comment at: test/tools/llvm-strings/file-filename.test:10
+CHECK:      [[FILE]]: abcd
+CHECK-NEXT: [[FILE]]: hijk
----------------
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.


================
Comment at: test/tools/llvm-strings/help.test:12
+CATEG: General options:
+CATEG: Generic Options:
 CHECK: @FILE
----------------
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.


================
Comment at: test/tools/llvm-strings/length.test:13
+RUN: llvm-strings -n 4 %t | FileCheck --check-prefix CHECK-4 %s --implicit-check-not={{.}}
+RUN: llvm-strings -n 5 %t | FileCheck --check-prefix CHECK-5 %s --implicit-check-not={{.}}
+
----------------
What if `-n 6` or `-n -1` or `-n foo`?


================
Comment at: test/tools/llvm-strings/version.test:1
+# Show that --version works for llvm-strings.
+
----------------
I.d probably just put this into `help.test`. What do you think?


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66015/new/

https://reviews.llvm.org/D66015