[UNIT] Add test case code for posix module coverage improve: from 23% to 50% #4623
Conversation
pugong2019
commented
Sep 13, 2025
pugong2019
commented
- Use LLM to generate the test case code for posix module
- Totally generate 108 cases
- Posix moudle's coverage improve from 23% to 50.7%
pugong2019
requested review from
loganek,
lum1n0us,
no1wudi,
TianlongLiang,
wenyongh,
xujuntwt95329 and
yamt
as code owners
lum1n0us
added
the
ai-assistant
pugong2019
force-pushed
the
AI_WAMR_2.4.1_Coverage
branch
from
462192c to
c8cee3c
Compare
| os_end_blocking_op(); | ||
| blocking_op_started = false; | ||
|
|
||
| // Test that ending again is safe (should be idempotent) |
There was a problem hiding this comment.
why should it be idempotent?
my impression is that this file is testing what the current implementation happens to be.
we should test the documented/defined functionality instead, IMO.
There was a problem hiding this comment.
we should test the documented/defined functionality instead, IMO.
It will be in the next phase. Currently, this task is focusing on covering the code.
There was a problem hiding this comment.
my point is that having these tests, especially with these comments, can give readers a wrong impression that it's a part of the API contract.
do we have any policy about ai-generated code? |
❓ What kind of policy are we seeking? |
the safest one is "do not accept any ai-generated code". |
pugong2019
force-pushed
the
AI_WAMR_2.4.1_Coverage
branch
from
c8cee3c to
8a44eb2
Compare
|
🤔 I am not sure about "any ai-generated code". AI assistants, like VSCode Copilot, GPT, and others, are helping us in various ways in our daily work. It's hard to determine which part of a PR is AI-generated, which part is typed by a human without referring to any AI generation, and which part is copied/pasted by a human from AI-generated content. This makes review very important and might require some additional rules for such PRs regarding copyright considerations? (personally, Definitely no idea) |
|
Is this the concern mentioned above? https://www.bloomberglaw.com/external/document/X4H9CFB4000000/copyrights-professional-perspective-ip-issues-with-ai-code-gener |
pugong2019
force-pushed
the
AI_WAMR_2.4.1_Coverage
branch
from
8a44eb2 to
99c8714
Compare
… 50% Signed-off-by: Gong Pu <[email protected]>
pugong2019
force-pushed
the
AI_WAMR_2.4.1_Coverage
branch
from
99c8714 to
e387c2a
Compare
| ${POSIX_PLATFORM_DIR}/posix_file.c | ||
| ${POSIX_PLATFORM_DIR}/posix_socket.c | ||
| ${POSIX_PLATFORM_DIR}/posix_blocking_op.c | ||
| ) |
There was a problem hiding this comment.
POSIX_SOURCEF_FILES isn't necessary. should be removed.
| set(unit_test_sources | ||
| ${POSIX_TEST_SOURCE} | ||
| ${POSIX_SOURCE_FILES} | ||
| ${WAMR_RUNTIME_LIB_SOURCE} |
There was a problem hiding this comment.
WAMR_RUNTIME_LIB_SOURCE includes POSIX_SOURCE_FILES. L44 should be removed.
| # Create test executable | ||
| add_executable(posix_platform_test ${unit_test_sources}) | ||
| enable_testing() | ||
| include(GoogleTest) |
There was a problem hiding this comment.
L50 and L51 are not necessary. unit_common.cmake includes that part.
| cmake_minimum_required(VERSION 3.14) | ||
|
|
||
| project(test-posix-platform) | ||
|
|
There was a problem hiding this comment.
Need to consider how to manage situations where the host(or targeting) system isn't POSIX-like. My poor suggestion is something like:
if(UNIX AND NOT APPLE)
message(STATUS "This is a POSIX-like system (Linux or similar).")
elseif(APPLE)
message(STATUS "This is macOS, which is POSIX-compliant.")
else()
message(STATUS "This is not a POSIX-like system.")
endif()| add_definitions(-DRUN_ON_LINUX) | ||
|
|
||
| set(WAMR_BUILD_LIBC_BUILTIN 1) | ||
| set(WAMR_BUILD_LIBC_WASI 1) |
There was a problem hiding this comment.
no need to open both. Or even any one.
| }; | ||
|
|
||
| // Step 3: Blocking Operations Tests for Coverage Improvement | ||
|
|
There was a problem hiding this comment.
There are ~6 functions in core/shared/platform/common/posix/posix_blocking_op.c, and the cases focus only on 2 of them. Since we aim to expand coverage quickly, it's preferable to create a few cases for each function rather than numerous cases for just one function.
i think there are two separate things:
your point about review importance is for the former, right? |
yes. (i only glanced at the article) |
which LLM and what contract? i've heard some LLM providers offer users some protections. eg. https://blogs.microsoft.com/on-the-issues/2023/09/07/copilot-copyright-commitment-ai-legal-concerns/
where/how can i see the number? |
| else { | ||
| EXPECT_EQ(__WASI_ENOTSUP, result); | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
IMM,
static __wasi_errno_t wasi_clockid_to_clockid(__wasi_clockid_t in, clockid_t *out)andstatic __wasi_timestamp_t timespec_to_nanoseconds(const struct timespec *ts)should be called directly rather than through other functions.- It seems the cases attempt to cover edge scenarios but mostly fall short. A quick way to cover corners is to prepare values based on the argument types, such as using 0, negative values, positive values, INT32_MIN, and INT32_MAX for int32_t types. However, this approach resembles using a test plan.
- If increasing coverage is not the plan, perhaps fewer cases will suffice.
|
|
||
| // Test PROCESS_CPUTIME_ID clock if supported | ||
| __wasi_errno_t result = | ||
| os_clock_res_get(__WASI_CLOCK_PROCESS_CPUTIME_ID, &resolution); |
There was a problem hiding this comment.
The result will be influenced by the compilation flag CLOCK_PROCESS_CPUTIME_ID. How should we address that? It is better not enable both assertions at L38 and L41 at the same time.
| } | ||
|
|
||
| // Test THREAD_CPUTIME_ID clock if supported | ||
| result = os_clock_res_get(__WASI_CLOCK_THREAD_CPUTIME_ID, &resolution); |
|
|
||
| // Test PROCESS_CPUTIME_ID clock | ||
| __wasi_errno_t result = | ||
| os_clock_time_get(__WASI_CLOCK_PROCESS_CPUTIME_ID, 0, &time); |
|
|
||
| // Test THREAD_CPUTIME_ID clock | ||
| __wasi_errno_t result = | ||
| os_clock_time_get(__WASI_CLOCK_THREAD_CPUTIME_ID, 0, &time); |
| rmdir(test_dir); | ||
| } | ||
| }; | ||
|
|
There was a problem hiding this comment.
better have some comments to explain the purpose of each test and any assumptions made
| EXPECT_GT(strlen(buffer), 0); | ||
|
|
||
| // Should contain RSS information | ||
| EXPECT_NE(nullptr, strstr(buffer, "RSS")); |
There was a problem hiding this comment.
the result depends on WASM_ENABLE_MEMORY_PROFILING. Need a way to handle it.
lum1n0us
changed the title
