Page MenuHomePhabricator

[flang] Move External IO tests to use GTest

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



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: 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.


[nit] Perhaps add the original comment here: 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/ 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 Michael's fix came a few commits later: @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 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.