Page MenuHomePhabricator

unittests: Use yaml2obj as a library instead of an external process
ClosedPublic

Authored by labath on Aug 8 2019, 6:31 AM.

Details

Summary

Recently, yaml2obj has been turned into a library. This means we can use
it from our unit tests directly, instead of shelling out to an external
process. This patch does just that.

Diff Detail

Repository
rL LLVM

Event Timeline

labath created this revision.Aug 8 2019, 6:31 AM
jdoerfert resigned from this revision.Aug 8 2019, 7:21 AM
JDevlieghere accepted this revision.Aug 8 2019, 12:39 PM
This revision is now accepted and ready to land.Aug 8 2019, 12:39 PM
aadsm added a comment.Aug 8 2019, 7:04 PM

I can see the appeal of having the contents next to the logic that is testing it, but I'm somewhat concerned for the cases where it includes +1000 lines of YAML in the test file. I think for those cases it might make sense to consider these fixtures and be in their own file?

labath updated this revision to Diff 214324.Aug 9 2019, 12:35 AM
  • put back the line entry test input into an external file
labath edited reviewers, added: sgraenitz; removed: espindola, jdoerfert.Aug 9 2019, 12:45 AM
labath added a subscriber: sgraenitz.

I can see the appeal of having the contents next to the logic that is testing it, but I'm somewhat concerned for the cases where it includes +1000 lines of YAML in the test file. I think for those cases it might make sense to consider these fixtures and be in their own file?

Yeah, I was wondering about that myself.... I can easily put back the inputs into the files -- that is independent of how we invoke yaml2obj. Right now, I've put the line table test back into an external file, but I've kept the other test as they are because I think they are of reasonable size (+@sgraenitz, if he has any thoughts on the symtab test). Overall, I think there's still some work to be done to make these tests really understandable. It's definitely better than having no tests, but for the line table test for instance, it's impossible to tell what input actually is from looking at the test data (regardless of which file it is in). It might be better overall to rewrite this input in assembly, as that would be shorter and more readable. Though it would require us to set up the infrastructure to run the assembler from a unit test...

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptAug 20 2019, 5:30 AM