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.
|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?
I don't necessarily like this, but can't think of an alternate. So, LGTM.
- 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
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
|16 ↗||(On Diff #248115)|
Agreed, it's not ideal, but I think it is required.
Couple of really nit-picky comments but overall LGTM.
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?
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?
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?
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.
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.
SGTM. Worst case, we can easily fix this if it gets irritating in future.
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:
I do not have an strong leanings, but it seems like #2 can be less confusing. WDYT?