This is an archive of the discontinued LLVM Phabricator instance.

[libc] [UnitTest] Add Matchers
ClosedPublic

Authored by abrachet on Mar 2 2020, 3:42 PM.

Details

Summary

This patch adds gtest-like matchers and EXPECT|ASSERT_THAT macros. It also adds matchers Succeeds and Fails and has examples using these in test/src/signal/sigaddset_test.cpp.

Diff Detail

Event Timeline

abrachet created this revision.Mar 2 2020, 3:42 PM
gchatelet added inline comments.Mar 3 2020, 12:09 AM
libc/utils/testutils/StreamWrapper.h
16

It would be nice to explain the purpose of this class.

abrachet updated this revision to Diff 248101.Mar 3 2020, 9:38 PM
  • Added comment explaining purpose of StreamWrapper
  • Removed pointless default initialization of StreamWrapper::OS
abrachet marked an inline comment as done.Mar 3 2020, 9:38 PM
gchatelet added inline comments.Mar 3 2020, 11:57 PM
libc/utils/testutils/StreamWrapper.h
16

Thx. There's a typo on indirection.

sivachandra added inline comments.Mar 4 2020, 11:07 AM
libc/utils/UnitTest/Matchers.h
1 ↗(On Diff #248115)

I didn't go look what gtest's suggested organization is, but instead of putting all matchers in Matchers.h, does it make sense to put the ErrnoSetterMacther class in its own header file?

16 ↗(On Diff #248115)

Why not enclose in __llvm_libc::testing? To avoid verbosity?

48 ↗(On Diff #248115)

These names are generic enough that some kind of specificity would be nice? I am not able to think of an alternate. May be ErrnoSetterSucceeds and ErrnoSetterFails?

libc/utils/testutils/StreamWrapper.h
2

I don't necessarily like this, but can't think of an alternate. So, LGTM.

abrachet updated this revision to Diff 248390.Mar 4 2020, 9:44 PM
  • Rename Matchers.h to ErrnoSetterMatcher.h
  • Moved Fails and Succeeds behind namespace ErrnoSetterMatcher so they can be easily shorted with using
  • Removed some newlines between EXPECT_THAT's in sigaddset_test.cpp
  • Put ErrnoSetterMatcher in namespace __llvm_libc::testing
abrachet marked 7 inline comments as done.Mar 4 2020, 9:48 PM
abrachet added inline comments.
libc/utils/UnitTest/ErrnoSetterMatcher.h
54

These names are generic enough that some kind of specificity would be nice? I am not able to think of an alternate. May be ErrnoSetterSucceeds and ErrnoSetterFails?

What about this set up? I think it is nice to be able to use a using and refer to these as just Succeeds and Fails for brevity. No strong preference happy to call these ErrnoSetterSucceeds/Fails

libc/utils/UnitTest/Matchers.h
16 ↗(On Diff #248115)

Good call.

libc/utils/testutils/StreamWrapper.h
2

Agreed, it's not ideal, but I think it is required.

sivachandra accepted this revision.Mar 4 2020, 10:37 PM
sivachandra marked an inline comment as done.

Couple of really nit-picky comments but overall LGTM.

libc/utils/UnitTest/ErrnoSetterMatcher.h
14

Normally, we should not use parts of the libc to build its testing infrastructure. This change is not doing anything of that sort, but having this include here gives an impression that we are using the libc to build the test infrastructure. So, may be add a comment of some kind to make it absolutely clear?

54

LGTM

57

Sorry I missed pointing out earlier: why not make the order of args to Fail and Succeeds the same? Even to the constructor of ErrnoSetterMatcher above?

This revision is now accepted and ready to land.Mar 4 2020, 10:37 PM
abrachet marked 4 inline comments as done.Mar 4 2020, 11:25 PM
abrachet added inline comments.
libc/utils/UnitTest/ErrnoSetterMatcher.h
14

Yes I was definitely weary about this. I had originally planned to put this file in test/ not utils/UnitTest but couldn't find a good place. Maybe making a common directory in test? So in libc/test/common/ErrnoSetterMatcher.h? Does that sound better?

57

I agree it is confusing.

The idea here was that for a failure errno is always different, and it would be awkward to do Fails(/*ExpectedReturn*/-1, EINVAL) rather than Fails(EINVAL). Likewise for Success(/*ExpectedErrno*/0, /*ExpectedReturn*/0) is awkward because errno should always be 0. Now that I think about it, maybe Success shouldn't take an errno parameter at all I can't imagine it ever being useful.

My plan is for these to be able to take matchers of their own in a future patch. I could in this patch have these take matchers and create an ErrnoIs/ReturnIs if you think the ordering of arguments is confusing enough to warrant this.

sivachandra marked 2 inline comments as done.Mar 5 2020, 8:42 AM
sivachandra added inline comments.
libc/utils/UnitTest/ErrnoSetterMatcher.h
14

No, leave it as is but add a comment. We want test/... to reflect the directory tree of the rest of the repo. If we really want a separate place, then may be utils/testmatchers. However, it feels like we should wait for more examples before we move things around.

57

SGTM. Worst case, we can easily fix this if it gets irritating in future.

PaulkaToast accepted this revision.Mar 5 2020, 12:56 PM
abrachet updated this revision to Diff 248617.Mar 5 2020, 3:36 PM

Added a comment explaining the inclusion of internal llvm-libc header in ErrnoSetterMatcher.h

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMar 5 2020, 3:53 PM
sivachandra added inline comments.Jul 16 2020, 11:55 PM
libc/utils/UnitTest/ErrnoSetterMatcher.h
14

I am beginning to feel that your suggestion is more appropriate. For, the current arrangement breaks a layering rule: a part of utils depends on src. We do not want utils to depend on src.

To keep the directory structure of test match that of the the libc tree, we can take one of these two options:

  1. Move testutils up out of utils in to a top level directory of its own.
  2. Move just this file into test and not nest it under test/common.

I do not have an strong leanings, but it seems like #2 can be less confusing. WDYT?

abrachet marked an inline comment as done.Jul 17 2020, 11:43 AM
abrachet added inline comments.
libc/utils/UnitTest/ErrnoSetterMatcher.h
14

No strong opinion either. I agree #2 is less confusing and slightly simpler I think.