Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add unit tests for the tid allocator #2519

Merged
merged 1 commit into from
Sep 4, 2023

Conversation

loganek
Copy link
Collaborator

@loganek loganek commented Aug 30, 2023

Also, as part of the PR I've added simple infrastructure to add more unit tests in the future. At the moment tests are only executed on Linux, but can be extended to other platforms if needed.

I'd like to get feedback on the approach for unit tests; we have extensive e2e tests (wasi, spec) but I think ability to run unit tests would allow us to test some of the smaller functionality quicker and make testing more granular.

I did use https://github.com/google/googletest/ as a framework; seems like it's quite popular in the industry, and nicely integrates with CMake. I didn't do any major research though so I'm open for suggestions. The major limitation is the fact that GTest requires C++ compiler, but not sure if that's an issue for testing infrastructure.

@loganek loganek force-pushed the loganek/unit-tests branch 6 times, most recently from 6b89ac9 to 13c48e3 Compare August 30, 2023 12:11
@@ -8,8 +8,14 @@

#include "platform_common.h"

#ifdef __cplusplus
extern "C" {
#endif
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can't googletest test plain C code? (just asking, i'm not familiar with googletest.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, gtest is implemented fully in C++, therefore test files must be C++ files as well.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI, i have been using cmocka for the other C project on macOS and ubuntu w/o any major issues.
(i'm not saying cmocka is better than other framework. actually i dunno.
i'm just saying i can share my experience.)

sample code: https://github.com/yamt/toywasm/blob/master/test/test.c

its output:

[==========] tests: Running 8 test(s).
[ RUN      ] test_leb128
[       OK ] test_leb128
[ RUN      ] test_endian
[       OK ] test_endian
[ RUN      ] test_functype
[       OK ] test_functype
[ RUN      ] test_idalloc
[       OK ] test_idalloc
[ RUN      ] test_timeutil
[       OK ] test_timeutil
[ RUN      ] test_timeutil_int64
[       OK ] test_timeutil_int64
[ RUN      ] test_list
[       OK ] test_list
[ RUN      ] test_xstrnstr
[       OK ] test_xstrnstr
[==========] tests: 8 test(s) run.
[  PASSED  ] 8 test(s).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for sharing; I don't have a strong opinion, I used gtest because it nicely integrates with CMake, is quite widely used, well documented, and I'm quite familiar with it. But let's see if others have any preferences, I don't mind moving to other framework.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one negative thing in cmocka is that its "mocking" functionality assumes ld --wrap and thus doesn't work on macOS. (i simply don't use the mocking functionality.)

Copy link
Contributor

@eloparco eloparco Aug 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've used gtest in the past for c code. There are some annoyances, like having to explicitly cast the return type of a malloc to compile (for instance, char* c = (char*)malloc(10);). From what I remember it has no (great) support for mocking in c.
For the rest, I don't recall any meaningful constraints.

class TidAllocatorTest : public ::testing::Test
{
protected:
void SetUp() override
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
void SetUp() override
void TidAllocatorTest()

I think constructors/destructors are the preferred way to do the same https://github.com/google/googletest/blob/main/docs/faq.md#should-i-use-the-constructordestructor-of-the-test-fixture-or-setupteardown-ctorvssetup

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll stick to setup/teardown, mainly because of this:

in the body of a constructor (or destructor), it's not possible to use the ASSERT_xx macros. Therefore, if the set-up operation could cause a fatal test failure that should prevent the test from running, it's necessary to use abort and abort the whole test executable, or to use SetUp() instead of a constructor.

@wenyongh wenyongh merged commit 8c2dc1d into bytecodealliance:main Sep 4, 2023
368 checks passed
victoryang00 pushed a commit to victoryang00/wamr-aot-gc-checkpoint-restore that referenced this pull request May 27, 2024
Add simple infrastructure to add more unit tests in the future. At the moment tests
are only executed on Linux, but can be extended to other platforms if needed.

Use https://github.com/google/googletest/ as a framework.
@loganek loganek deleted the loganek/unit-tests branch June 10, 2024 12:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants