Port external-io test to use GTest. Remove Runtime tests directory. Rename RuntimeGTest directory to Runtime.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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 | ||
---|---|---|
9 | [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.
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!
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.
The pre-merge check had a small clang-format suggestion for ExternalIOTest.cpp. Otherwise, LGTM.
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)
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.
[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.