[PATCH] D97349: [flang] Change existing runtime tests to use gtest
Andrzej Warzynski via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Feb 24 05:17:00 PST 2021
awarzynski added a comment.
Hi @ashermancinelli ,
Many thanks for working on this! This is a great starting point, but I agree that it is too large to be merged as is. I suggest splitting this into smaller parts. I think that introducing a dedicated directory for GTest port is a fair compromise during the transition period.
A few thoughts:
- `CharacterTest.cpp` looks fairly small and could be a good starting point. Similarly for `ListInputTest.cpp`.
- Currently `CharacterTest.cpp` is not very GTest-idiomatic. I see 3 methods being tested there (`CharacterAppend1`, `CharacterPad1`, `CharacterCompareScalar1`). I think that introducing a dedicated test case for each would make a lot of sense.
- In general I think that having a lot of small test cases would be cleaner than a handful of bigger cases that cover multiple APIs.
- Do all tests need `StartTests();` and `EndTests()` (thinking about `CharacterTest.cpp` again)? Also, shouldn't that be included in some test fixture <https://github.com/google/googletest/blob/master/docs/primer.md#test-fixtures-using-the-same-data-configuration-for-multiple-tests-same-data-multiple-tests>.
Otherwise this makes a lot of sense to me. I've also added a few other reviewers .
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D97349/new/
https://reviews.llvm.org/D97349
More information about the llvm-commits
mailing list