Currently we don't test all core note types that are defined in
getCoreNoteTypeName in ELFDumper.cpp.
Also we don't have a test for an unknown core note type.
This patch fixes it.
Paths
| Differential D87453
[llvm-readobj/elf][test] - Test all core note types properly. ClosedPublic Authored by grimar on Sep 10 2020, 7:07 AM.
Details Summary Currently we don't test all core note types that are defined in Also we don't have a test for an unknown core note type. This patch fixes it.
Diff Detail
Event TimelineComment Actions Emm. I am a bit less sure about the value enumerating every value. It just repeats the getCoreNoteTypeName list in the test file. Two or three values should work, right?
Comment Actions
I believe all values should be tested at least somewhere. We had a situation previously (for file format names) where we tested all values in lib/Object unit test (ELFObjectFileTest.cpp) This is the same what we do for other things. E.g. we normally test all possible relocations, dynamic tags, etc. Comment Actions 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. This revision is now accepted and ready to land.Sep 14 2020, 3:18 AM Comment Actions
It takes 1.63s in debug configuration for me. (i7-4790K@4.2 Ghz, windows 8.1 64, 32 Gb Ram, SSD). -- Testing: 1 tests, 1 workers -- PASS: LLVM :: tools/llvm-readobj/ELF/note-core.test (1 of 1) Testing Time: 1.63s Passed: 1 Is it OK or too much? Closed by commit rGf4eb94e1db88: [llvm-readobj/elf][test] - Test all core note types properly. (authored by grimar). · Explain WhySep 14 2020, 4:27 AM This revision was automatically updated to reflect the committed changes. grimar marked an inline comment as done.
Revision Contents
Diff 291538 llvm/test/tools/llvm-readobj/ELF/note-core.test |
Not consistent with other Check where a period is present