[PATCH] D87453: [llvm-readobj/elf][test] - Test all core note types properly.

James Henderson via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon Sep 14 03:18:09 PDT 2020


jhenderson accepted this revision.
jhenderson added a comment.
This revision is now accepted and ready to land.

I think there's probably some value testing every value, as there is a chance that a future refactor could lead to a bug where there is ambiguity between a CORE note value and another variety of note, for example. Having a test for each individual value ensures that any such bug would be picked up. However, I'm not dead set on this idea, and can see counter-arguments to it. LGTM, but happy to defer to others opinion on whether testing each value is atually necessary.

If you haven't already, could you run this test on Windows and make sure it doesn't take an unreasonable amount of time (since there are a lot of process executions, which are expensive on Windows), please. In theory, the test could be written with a single large input testing each individual value. This would make the test check/input bigger, but would at least avoid 150 or so process executions within a single test.


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

https://reviews.llvm.org/D87453



More information about the llvm-commits mailing list