Move character tests to gtest, according to reviews from revision
D97349. Create a new temporary directory parallel to old runtime
unittests directory to facilitate the transition. Once patches for all
tests have been accepted using GTest, the old directory may be removed.
The new directory is required because LLVM's CMake unit test
infrastructure requires that the test depends on all source files in
the CMAKE_CURRENT_SOURCE_DIR directory.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
I no longer modify anything in the old runtime tests directory, so this patch should be much more straightforward.
Hi @ashermancinelli ,
Thank you for working on this! I quickly scanned through the test coverage is identical to flang/unittests/Runtime/character.cpp. What follows are some ideas for fine-tuning.
- Please don't hesitate to improve test coverage if you see some low hanging fruits :)
- Please remember about file headers
- The logic in RuntimeTesting.cpp is a bit complex - is there any chance that that's tested too? (in particular, CatchCrash, perhaps by triggering a call to Crash)
- I am also thinking that since procedures from character.cpp don't crash (at least the ones that you test), we don't really need CatchCrash just yet (though it would be great to see it being tested). Perhaps the test fixture could be simplified?
- IMO we should avoid duplicating code/tests. I recommend deleting similar tests from flang/unittests/Runtime.
More comments inline.
flang/unittests/RuntimeGTest/CharacterTest.cpp | ||
---|---|---|
18 |
| |
24 | IIUC, CharacterPad1 does not affect xlen, so this check could be moved before L22. | |
25–27 | Perhaps: EXPECT_NE(x[limit], `\0`) << "x[" << limit << "]='" << x[limit] << "'\n"; IMHO this would be clearer. | |
29 | IIUC, at this point x = abcDE and this memcpy simply overwrites x with similar value? I think that std::memcpy either succeeds or we get UB. So this ASSERT_FALSE does not really add much, does it? | |
48 | [nice-to-have] CharacterCompareScalar2, CharacterCompareScalar3, CharacterCompareScalar4 :) | |
53 | Why not use x and y instead of buf[0] and buf[1], respectively? Doesn't ASSERT_EQ() << behave like operator<< for std::ostream. So there should be no need to convert x and y to character arrays? Am I missing something? | |
64–68 | [nit] Personally I like to put ASSERT_ and EXPECT_ in the main test method (like this one). IMO it makes it much clearer what is actually being tested. But it's a matter of preference! | |
flang/unittests/RuntimeGTest/RuntimeTesting.h | ||
17 ↗ | (On Diff #326149) | This is currently only used in RuntimeTesting.cpp. Perhaps move there? |
Thank you for the review! It appears that many of your comments are due to lack of attention to detail when moving the test bodies over from the old runtime tests directory to this one (eg your comments on lines 28 and 52) - I'll revisit all of your comments and see where I can improve upon the old tests as well.
flang/unittests/RuntimeGTest/CharacterTest.cpp | ||
---|---|---|
64–68 | I agree, but do you have a suggestion for how to move the assertions/expectations into the body of the test without code repetition due to the parameter swapping? I'm happy to field suggestions on this one :) | |
flang/unittests/RuntimeGTest/RuntimeTesting.h | ||
17 ↗ | (On Diff #326149) | Correct, however this will be used in other tests as they are moved over from the old runtime tests so I think it could either remain here or be removed altogether. |
flang/unittests/RuntimeGTest/CharacterTest.cpp | ||
---|---|---|
29 | memcmp, not memcpy |
flang/unittests/RuntimeGTest/CharacterTest.cpp | ||
---|---|---|
25–27 | I agree, but this should be EXPECT_EQ(x[limit], '\0') << "x[" << limit << "]='" << x[limit] << "'"; correct? |
flang/unittests/RuntimeGTest/CharacterTest.cpp | ||
---|---|---|
25–27 | +1 | |
29 | :facepalm: Thank you! | |
64–68 | IMO this is a borderline situation. Yes, code re-use is nice. But so is making code & tests easier to follow. Which should we prioritize? Also, I left a comment for DoCharacterComparison - I think that that method can be simplified, so there won't be that much code to re-use. I marked this as a [nit] as IMO it's quite subjective. I'm happy with whatever you decide or propse! | |
flang/unittests/RuntimeGTest/RuntimeTesting.h | ||
17 ↗ | (On Diff #326149) | Then I suggest removing (so that the patch only contains the required stuff) |
flang/unittests/RuntimeGTest/CharacterTest.cpp | ||
---|---|---|
48 | I don't think this would be too hard with some template shenanigans... I'll add it in the next patch. |
Sure - it seems the existing method of crash recovery (exceptions) is the only way to go here since crash std::aborts, so we'll have to redo this part anyways.
- Templated character tests over all char types
- Removed unneeded fixture
- Documentation and better naming
flang/unittests/RuntimeGTest/CharacterTest.cpp | ||
---|---|---|
97–98 | An alternative solution to the CreateParameters function would be a static tuple of type tuple<vector<PARAMETER<char>>, vector<PARAMETER<char16_t>>, ...>. This way, initializing parameters would just be std::get<CHAR>(AnswerKey) or something similar. If anyone thinks this or another solution would be better than what I've used here, please let me know. |
Thank you for addressing my comments @ashermancinelli ! This looks really good and provides more test coverage than flang/unittests/Runtime/character.cpp, that's great! Before this is merged, could you delete the original test file?
I've left a few more suggestions, but nothing major and mostly nits.
flang/unittests/RuntimeGTest/CharacterTest.cpp | ||
---|---|---|
12–14 | A couple of nits: [nit1] exhaustive testing will be done in Fortran. - I would leave this out (it's a comment about things happening elsewhere in the codebase - it's unlikely that we will remember to update this comment once that exhaustive testing happens) [nit2] Similar testing infrastructure also exists in the parallel directory - this won't be needed if tests are not duplicated, right? | |
27 | [nit] I would be tempted to separate this and tests for CharacterCompareScalar{n} with a big comment, e.g.: //----------------------------------------------------------------------- /// Tests (and infra) for CharacterAppend1 and CharacterPad1 //------------------------------------------------------------------------ | |
35 |
// Verify that the limit is not exceeded xLen = RTNAME(CharacterAppend1)(x, limit, xLen, "123456789", 9); ASSERT_EQ(xLen, limit) << "xLen " << xLen << ">" << limit; |
Updated documentation, added a new test case based on conversation in the previous diff, and removed old character tests.
LGTM, thank you!
flang/unittests/RuntimeGTest/CharacterTest.cpp | ||
---|---|---|
27 | Both files would be quite small - personally I'd keep it here. But I won't object if you prefer to split. | |
125–127 | Btw, I thought that we could get rid of this and now I see that we cannot. My bad, apologies! But I don't think that the code-duplication here is bad. IMO it actually makes this easier to read. But I appreciate that that's subjective :) |
Hi @ashermancinelli ,
I've just noticed that character.cpp has been updated in https://reviews.llvm.org/D97580 in the meantime. Would you be able to re-base?
Also, CC @klausler who touched that file and @jeanPerier who reviewed D97580. Apologies for missing you earlier!
I've added tests for scan and verify functions, and cleaned up the comparison tests. I think the std::get method is a bit cleaner than what we had before, but I would love to hear other opinions on this.
flang/unittests/RuntimeGTest/CharacterTest.cpp | ||
---|---|---|
198 | AFAIK, char_traits<>::length is exactly the same as strlen but I can't verify this on cppref. If someone can verify that would be great. | |
264–272 | The tests for VerifyX and ScanX are identical and could potentially be merged into one test case. I think this may increase the work needed to add test cases in the future so I kept them separate, but please feel free to suggest alternatives. |
Thank you for updating this, @ashermancinelli! You've increased the test coverage for Verify{N} and Scan{N} methods - that's great to see.
I agree with your design decisions and I don't see any obvious ways to improve this. IMHO, this implementation is relatively easy to follow - you've done a really good job here!
I've just realized one thing. ALL_CAPS is usually reserved for macros (though I couldn't find this in LLVM's coding style docs). When specifying type aliases with using, I believe that we should use CamelCase instead. That's based on what's seem to be most widely used inside Flang. So, for example,
- VerifyParameterTy instead of VERIFY_PARAMETER
- VerifyFuncTy instead of VERIFY_FUNC
Apologies for not raising this earlier. I really liked your original approach, but we should follow the existing precedent in the project (I couldn't find any explicit notes on using, neither here nor here).
Few more comments/suggestions inline.
flang/unittests/RuntimeGTest/CharacterTest.cpp | ||
---|---|---|
58 | Perhaps ComparisonFuncTy? | |
62 | [nit] Would it be worth adding a comment here to improve visibility e.g. The list of CharacterCompareScalar{N} functions tested here? It's the key bit, isn't it? Same in other places. | |
71 | [nit] PARAMETER --> PARAMETERS (same in other places) | |
154 | ScanParameterTy instead? | |
198 | Yes, that's how I read this. |
ALL_CAPS is usually reserved for macros...
but we should follow the existing precedent in the project...
I'm used to camel case for template args as well and I too searched the documentation for guidance, but not finding a clear policy I tried to follow the conventions in the file being tested:
template <typename CHAR, bool ADJUSTR> static void Adjust(CHAR *to, const CHAR *from, std::size_t chars) {
Do you think we should break from conventions in the testee? Personally, I too prefer camelcase, but I'll defer to you on this one 🙂
In general, we've been using ALL_CAPS for template parameters and CamelCase for type names.
flang/unittests/RuntimeGTest/CharacterTest.cpp | ||
---|---|---|
222 | I suggest: using VerifyParameter = .... | |
227 | This should be verifyParameters. It might be more readable to give this type a name if you can come up with a good one. |
I've updated the revision - thanks for your comments @awarzynski and @tskeith! Please let me know what you think of the naming conventions I went with in this revision.
flang/unittests/RuntimeGTest/CharacterTest.cpp | ||
---|---|---|
154 | I like that 👍 | |
222 | Thanks for the suggestions! |
Updated naming conventions and documentation thanks to comments and suggestions from @awarzynski and @tskeith.
A couple of nits:
[nit1] exhaustive testing will be done in Fortran. - I would leave this out (it's a comment about things happening elsewhere in the codebase - it's unlikely that we will remember to update this comment once that exhaustive testing happens)
[nit2] Similar testing infrastructure also exists in the parallel directory - this won't be needed if tests are not duplicated, right?