Page MenuHomePhabricator

[libc] Replace the use of gtest with a new light weight unittest framework.
ClosedPublic

Authored by sivachandra on Jan 14 2020, 4:14 PM.

Details

Summary

Header files included wrongly using <...> are now included using the
internal path names as the new unittest framework allows us to do so.

Diff Detail

Event Timeline

sivachandra created this revision.Jan 14 2020, 4:14 PM
abrachet added inline comments.Jan 15 2020, 1:19 PM
libc/src/sys/mman/munmap.h
12

While we are here, the other comments end with a period so this one should too.

libc/test/src/string/strcat_test.cpp
21

Shouldn't we keep this as just ASSERT_EQ, the streq doesn't make much sense because these should be the same pointers. I think right now ASSERT_EQ with char* will not work but see my other comments.

libc/utils/UnitTest/Test.cpp
43

Maybe a using llvm_libc::testing::Test; and shorten these to Test::Cond_EQ?

48

What about llvm::withColor(llvm::errs(), llvm::raw_ostream::RED); Even if you don't like the color, all of these should be printing to stderr.

133

What if addTest added to a vector that way we can easily change the order the tests are executed for like in --gtest_shuffle

151

for (Test *T = Start; T; T = T->Next, ++TestCount)

And then the

Test *T = Start;
 if (T == nullptr)
   return 0;

can be removed along with the ++TestCount right below this comment.

175

What about !!FailCount?

178–184

You can create

template <typename T>
bool Test::test(...) {
   return ::test(...);
}

and then instantiate each of these

template Test::test<int>(RunContext &, ...);

But see previous comment I think we can do this differently.

libc/utils/UnitTest/Test.h
56–59

We can make this template call ::test(...) just here. But also It would be worth it to have enable_if specialization for std::is_pointer to call ::test<const void*>, and std::is_integral to call ::test<int64_t> and one for unsigned integrals to call use uint64_t so that we only need to have a couple instantiated in Test.cpp.

103–106

What do you think about making the ASSERT_*'s into if (!EXPECT_*) return?

sivachandra marked 10 inline comments as done.

Address comments.

libc/test/src/string/strcat_test.cpp
21

I have added a new ASSERT_PTREQ macro to test pointer equality. I previously had a specialization of ASSERT_EQ for void *. But, I think something like a PTREQ it is more useful as it can be used to compare any kind of pointer.

libc/utils/UnitTest/Test.cpp
43

Its affecting only this function so I have left it as is.

48

Streaming to llvm::errs() instead of llvm::outs().

133

Yeah, I did consider that. But, that means we will have a global non-POD which is usually not considered as a good practice. We can always reconsider this in case we want features like --gtest-shuffle.

175

Personally, I do not like the implicit nature of such a construct.

178–184

I do not think a declaration of a function specialization instantiates it. Am I misunderstanding something?

libc/utils/UnitTest/Test.h
56–59

This a very good suggestion to enable_if. But, I do not want to include <type_traits> here so I have now implemented an equivalent and used it to incorporate your suggestion. The benefit of using it is that the use of unsupported types leads to a compile-error which is more descriptive than a link-error (which was the case earlier.)

About calling ::test, since ::test is template function, we will have to define it in the header. We will then have to include some headers which we want to avoid.
About filtering signed integers to int64_t comparison etc., I want to avoid such implicit conversions so I have kept it as is.
About using std::is_pointer, I have added new EXPECT_PTREQ and friends. These macros cast the pointers to unsigned long long and do the comparison.

103–106

This is yet another good suggestion. Done now.

abrachet added inline comments.Jan 16 2020, 2:36 PM
libc/utils/UnitTest/Test.cpp
15

This include is unnecessary I think.

149

This one is still to outs

156

ditto

159

ditto

165

ditto

178–184

It instantiates if you write like template function<params_to_instantiate>(...);. Its used more often with class templates but it works with function templates as well. In llvm, I have seen it here.

libc/utils/UnitTest/Test.h
24

It looks like libcxx does remove_cv before these specializations. Which confuses me because

template <typename T> void f(T) {}
template <> void f<char>(char) {}
int main() {
  const volatile char c = 'c';
  f(c); // calls f<char>
}

But I figured I would mention it.

56–59

About calling ::test, since ::test is template function, we will have to define it in the header. We will then have to include some headers which we want to avoid.

Thats no different than what we have now we have the declaration of Test::test and it's definition in the cpp file. Just putting template <typename ValType> bool ::test(...); in the header file and then making Test::test call ::test makes it inline-able but more importantly its a little easier to follow I think.

About using std::is_pointer, I have added new EXPECT_PTREQ and friends. These macros cast the pointers to unsigned long long and do the comparison.

I think it is important that we make a IsPointerType specialization so we can just use EXPECT_EQ and be consistent with gtest. This will confuse people later I'm pretty sure. Also if we do the above suggestion this will be easy to make this specialization call ::test<unsigned long long>.

I just checked quickly and it looks like gtest actually prints to stdout which surprised me, I always figured it was to stderr. Do you think we should go back to stdout?

sivachandra marked 7 inline comments as done.

Address another round of comments.

I just checked quickly and it looks like gtest actually prints to stdout which surprised me, I always figured it was to stderr. Do you think we should go back to stdout?

TBF, I do not have any opinion here. For me, a message from a test run is all useful. So, its hard for me to see why something should be logged to stderr and something else to stdout. I have now put everything back to stdout because I actually think there could be tools out there which are probably only looking at stdout.

libc/utils/UnitTest/Test.cpp
149

The others were really an error message, so may be stderr was more appropriate. These are purely informational and hence I left them with stdout.

178–184

Ah, I never used this before. I have modified as you have suggested now.

libc/utils/UnitTest/Test.h
56–59

I don't have any opinion about this. I had previously thought I should limit the re-implementation of std types. But heck, we have already come this far so I have implemented everything as you have suggested.

abrachet accepted this revision.Jan 17 2020, 7:51 AM
abrachet added inline comments.
libc/test/config/linux/x86_64/syscall_test.cpp
12

Should we get rid of this include too? Maybe just assign to function pointer? Or we could implement std::is_same?

libc/test/src/string/strcat_test.cpp
35–36

I guess we don't really need these STREQ anymore.

This revision is now accepted and ready to land.Jan 17 2020, 7:51 AM
phosek accepted this revision.Jan 17 2020, 2:51 PM

LGTM

libc/utils/UnitTest/Test.cpp
119

Have you considered using a switch instead so that the compiler statically checks that you've handled all conditions? I'd expect it to also be more efficient.

150

This could also be a switch.

libc/utils/UnitTest/Test.h
96

Why not bool as a return value`?

sivachandra marked 5 inline comments as done.Jan 17 2020, 4:36 PM

I modified to use switch statements as suggested before submitting.

libc/test/config/linux/x86_64/syscall_test.cpp
12

Yes, it should go. I have kept it for now as I have a different plan for things like these. Will write about them in detail soon.

libc/test/src/string/strcat_test.cpp
35–36

No harm to keep them I guess :)

libc/utils/UnitTest/Test.h
96

The main function can then just return the result of runTests. I don't think bool return value disallows it, but I don't like the implicit nature of such conversions.

This revision was automatically updated to reflect the committed changes.