[PATCH] D64800: [llvm-readobj] - Stop using precompiled objects in file-headers.test

George Rimar via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Jul 17 10:45:06 PDT 2019


grimar added a comment.

In D64800#1589215 <https://reviews.llvm.org/D64800#1589215>, @jhenderson wrote:

> I feel look we've lost some test coverage here: a number of the checked fields have gone from being non-zero to being zero. This means that we're no longer testing that the property is actually read and printed correctly, so I think the YAML needs updating in these cases.


Let me clarify. Do you mean, for example, that for `COFF-ARM64`, `SectionCount` became `0`, but was non-zero before?
Such things is a result of YAML simplifications and was my intention actually. I tried to make YAMLs as minimal as possible to show
how the headers are dumped. Like now, `COFF-ARM64` check that when there is no sections then we print zero as expected, what is also a reasonable check I think.
I could just insert the unsimplified YAMLs instead, but that would make this test really huge and actually dirty. The binaries we have seems have too
much excessive content.  Also that anyways would not actually test the each possible field. Because for full testing of that we would need to have at least 2 tests I think,
like with 0 sections and with N sections to show the difference. That applies for each field.

I am OK to insert unmodified or partially simplified YAMLs if you want. (I do not think I am ready to make a huge amount of work to add complete tests for the each field though)
What kind of YAMLs would make you happy enough? :)

(Btw, my motivation of doing this patch was: I want to simplify another test by moving its parts to another places,
like this one and eliminate. Generally I found very usefull to have no binaries in the any inputs, removing them should allow us to significantly
cleanup and improve the existent tests and also the tools).


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

https://reviews.llvm.org/D64800





More information about the llvm-commits mailing list