Page MenuHomePhabricator

[flang] update character tests to use gtest
ClosedPublic

Authored by ashermancinelli on Feb 24 2021, 11:09 AM.

Details

Summary

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.

Diff Detail

Event Timeline

ashermancinelli requested review of this revision.Feb 24 2021, 11:09 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 24 2021, 11:09 AM

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
  • this can be declared outside the loop
  • both here and in the loop, 8 is a magic number that's re-used and that could be replaced with a variable
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?

ashermancinelli added a comment.EditedFeb 25 2021, 10:55 AM

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?

awarzynski added inline comments.Feb 25 2021, 11:58 AM
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.

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?

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.

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.

Wouldn't death tests work here?

ashermancinelli marked 5 inline comments as done.Feb 26 2021, 8:07 AM
  • 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
  1. There's quite a few checks in this test - would it be helpful to add a short comment highlighting _why_ or _what_ is validated?
  1. Would it make sense to add a corner-case test? For example:
// Verify that the limit is not exceeded
xLen = RTNAME(CharacterAppend1)(x, limit, xLen, "123456789", 9);
ASSERT_EQ(xLen, limit) << "xLen " << xLen << ">" << limit;
ashermancinelli added inline comments.Mar 1 2021, 1:12 PM
flang/unittests/RuntimeGTest/CharacterTest.cpp
27

If we're already breaking the tests up into smaller units, would we be better served to break tests into two files?

35

Both counts sound good to me!

Updated documentation, added a new test case based on conversation in the previous diff, and removed old character tests.

SouraVX retitled this revision from updated character tests to gtest to [flang] updated character tests to gtest.Mar 1 2021, 8:47 PM
awarzynski accepted this revision.Mar 2 2021, 10:56 AM

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

This revision is now accepted and ready to land.Mar 2 2021, 10:56 AM

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!

@awarzynski Sure! I'll add corresponding tests and have another patch up soon.

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.

@awarzynski

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:

flang/runtime/character.cpp:

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.

ashermancinelli marked 2 inline comments as done.EditedMar 7 2021, 3:54 PM

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.

Thank you for clarifying, @tskeith.

LGTM @ashermancinelli !

ashermancinelli retitled this revision from [flang] updated character tests to gtest to [flang] update character tests to use gtest.Mar 8 2021, 11:26 AM
ashermancinelli edited the summary of this revision. (Show Details)Mar 8 2021, 11:32 AM
ashermancinelli edited the summary of this revision. (Show Details)Mar 8 2021, 11:37 AM
This revision was automatically updated to reflect the committed changes.