[PATCH] D57877: [llvm-objcopy] Add few file processing directives

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: test/tools/llvm-objcopy/ELF/strip-symbol.test:9
+# RUN: echo "b.*" > %t-list.txt
+# RUN: echo " # empty symbol name with comment" >> %t-list.txt
+# RUN: llvm-objcopy --regex --strip-symbols %t-list.txt %t %t5
----------------
evgeny777 wrote:
> jhenderson wrote:
> > Why is this tested under strip-symbol and not the other cases?
> > 
> > Related aside: this is a good example of where unit testing should be present because really we're testing the symbol reading from file function not the switch behaviour.
> > Why is this tested under strip-symbol and not the other cases?
> 
> What's the point of doing this everywhere? We're using the same file parser for all cases
My point is more that we are testing a detail of the parser in a test that doesn't obviously have anything to do with that parser. Tests should test one feature or behaviour or it becomes hard to determine at a glance why a test is failing, or what behaviours or features are under test. I would deem that the parser is itself a feature, and should be tested separately (with separate tests to show that clients use it correctly).

Let's imagine the (admittedly somewhat contrived case) that we decide that the behaviour of --strip-symbols should change, e.g. to not allow comments any more. That would imply that this particular case should be removed from this test. However, given the current state of this patch, that would mean that we don't have any testing for comments in the normal parser.

Two options would be either to add the comment behaviour testing to each user of the parser or to write a test that tests comment handling explicitly. As noted above, this would naturally be a unit test, but given the current state of llvm-objcopy that is not possible. An alternative would be one or more lit tests called something like "symbols-from-file.test" or whatever.


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

https://reviews.llvm.org/D57877