This is an archive of the discontinued LLVM Phabricator instance.

[flang] Move External IO tests to use GTest
ClosedPublic

Authored by ashermancinelli on Jul 1 2021, 2:03 PM.

Details

Summary

Port external-io test to use GTest. Remove Runtime tests directory. Rename RuntimeGTest directory to Runtime.

Diff Detail

Event Timeline

ashermancinelli created this revision.Jul 1 2021, 2:03 PM
ashermancinelli requested review of this revision.Jul 1 2021, 2:03 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 1 2021, 2:03 PM

This is the final patch to complete the porting of runtime unit tests to GTest. It's an important milestone! @ashermancinelli, many thanks for seeing this through! I've compared flang/unittests/Runtime/ExternalIOTest.cpp and flang/unittests/Runtime/external-io.cpp - the semantics are identical and only the clang-tidy warnings need addressing before merging.

As for the failing CI tests - I can see that external-io is currently not being run on Windows: https://lab.llvm.org/staging/#/builders/179/builds/653/steps/7/logs/stdio. This suggests that this patch _exposes_ rather than _introduces_ these failures. I suspect that there is something Linux-specific about that test (file I/O?), but I can't see anything obvious. @Meinersbur set-up that Windows BuildBot worker - perhaps he'll have a suggestion? If we can't identify anything, GTEST_SKIP + bugzilla should be sufficient.

flang/unittests/Runtime/ExternalIOTest.cpp
8

[nit] Perhaps add the original comment here: https://github.com/llvm/llvm-project/blob/main/flang/unittests/Runtime/external-io.cpp#L1. Just to make sure that everything is preserved.

Sorry for the wait. Flang-OldUnit indeed currently does not run under ninja make-check because Windows executables have an .exe suffix (external-io.test.exe) that is not in lit's suffixes list. The CMake target otherwise unusual name is external-io.text (or external-io-slow) because

config.suffixes = [".test"]

in NonGTestUnit/lit.cfg.py. Glad that this is fixing this.

The Windows fails are fixed by D106726.

Update comments and formatting in response to feedback and clang-tidy.

Tests on Windows should be passing now.

According to Phabricator, your revision is based on https://reviews.llvm.org/rG56fa49878b71a1d92b9a93b586cff26829abe157. Michael's fix came a few commits later: https://reviews.llvm.org/rGcbad57613e769d0653e13cf877d80eae421b2314. @ashermancinelli could you rebase on top of main? That should address the CI failures, Thanks!

Rebase on main.

Still failing :( @Meinersbur, do you have any spare cycles to take a look?

The diff is still based on rG56fa49878b71 from one month ago, as the "commits" tab shows. The Semantics::unpack.f90 test has been fixed since then. How do you update the diff?

When applying locally, I don't see any errors.

  • Rebase diff
Meinersbur accepted this revision.Jul 28 2021, 11:59 PM

The pre-merge check had a small clang-format suggestion for ExternalIOTest.cpp. Otherwise, LGTM.

This revision is now accepted and ready to land.Jul 28 2021, 11:59 PM
awarzynski accepted this revision.Jul 29 2021, 1:26 AM

Thank you for your help Michael! Fixing those Windows failures would have been _really_ tricky without your input.

@ashermancinelli, many thanks for seeing flang/unittests/Runtime through :) With your refactoring, we have much better test coverage, a few bugs got fixed and the code itself is much more modern and neater. LGTM (modulo that clang-format suggestion)

Apply clang-format.

ashermancinelli marked an inline comment as done.Jul 29 2021, 12:35 PM

Thanks for the kind words @awarzynski! Glad I could help 🙂

Rebase to retrigger CI.

Rebase to retrigger CI.

The most recent failure in the CI is unrelated to this patch and was fixed in https://reviews.llvm.org/D107107. It is safe to merge this change.

This revision was landed with ongoing or failed builds.Jul 30 2021, 9:22 AM
This revision was automatically updated to reflect the committed changes.
flang/unittests/Runtime/tools.h