Page MenuHomePhabricator

[flang] Change existing runtime tests to use gtest
AbandonedPublic

Authored by ashermancinelli on Feb 23 2021, 5:12 PM.

Details

Summary

Updates the flang runtime unittests to use GTest. This patch should
likely not be accepted as-is, as there are temporarily two directories
for runtime tests such that new tests may be
written to use GTest without interfering with the old ones. Eventually,
old runtime tests directory should be removed.

The style guide indicates that exceptions should never be used,
however almost all of the the current runtime tests use exceptions.
This revision removes this need for exceptions.

I attempted to follow the style guide as closely as possible, however
some of the files flang/unittests/Runtime differ in style from other
unittest directories. In these cases, I leaned towards conforming with
the other existing unittests and not the precedent set by
flang/unittests/Runtime directory. For example, for file names I used
pascal case instead of kabob case, and test cases are in pascal case
instead of camel case.

Some tests require input and will likely not be able to remain in the
same directory as the other runtime tests since LLVM's CMake unittest
functions do not allow multiple targets in a directory (as discussed
on slack). Example is

This is my first revision, so please point out anything that doesn't
conform to best practices.

Diff Detail

Event Timeline

ashermancinelli requested review of this revision.Feb 23 2021, 5:12 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 23 2021, 5:12 PM
awarzynski added a comment.EditedFeb 24 2021, 5:16 AM

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.

Otherwise this makes a lot of sense to me. I've also added a few other reviewers for better visibility.

Thanks for the patch! Could you please clang-format the patch(this is will remove lot of Lint warning/noise) and ease up the review.
If you're using arcanist then that will automatically take care of formatting(provided clang-format should be in system path.) Or if you're using web UI then you may have to do it manually (git-clang-format HEAD~).
More details: https://llvm.org/docs/Phabricator.html

Thanks for the patch! Could you please clang-format the patch(this is will remove lot of Lint warning/noise) and ease up the review.

+1 (btw, with shift+a you can hide all the warnings)

ashermancinelli abandoned this revision.Feb 24 2021, 7:22 AM

Thanks for the patch! Could you please clang-format the patch(this is will remove lot of Lint warning/noise) and ease up the review.
If you're using arcanist then that will automatically take care of formatting(provided clang-format should be in system path.) Or if you're using web UI then you may have to do it manually (git-clang-format HEAD~).
More details: https://llvm.org/docs/Phabricator.html

Thank you for the note - I used arc like so:

$ arc diff a0839b14df6d
Linting...
 OKAY  No lint messages.
 LINT OKAY  No lint problems.
Running unit tests...

so it seems I'll have to clang-format manually.

  • Do all tests need StartTests(); and EndTests() (thinking about CharacterTest.cpp again)? Also, shouldn't that be included in some test fixture.

Yep! This seems like a better way to handle the runtime crashes. I'll have that for the next patch.