C++: Turn header variant tests that use PCH files into integration tests #19410
Conversation
These tests currently test a code path in the extractor that only exists for these tests. By turning them into integration tests, we actually use the code path that normal database creation uses.
There was a problem hiding this comment.
Pull Request Overview
This PR converts header variant tests using precompiled headers (PCH) into integration tests to exercise the normal database creation code path. Key changes include:
- Updating Microsoft PCH tests by adding simple C source files with appropriate header inclusion directives.
- Modifying Clang PCH tests to run as integration tests via a Python test script and multiple minimal C source files.
- Removing hacky test setups by ensuring that PCH creation and inclusion follow realistic code extraction flows.
Reviewed Changes
Copilot reviewed 36 out of 36 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| cpp/ql/integration-tests/header-variant-tests/microsoft-pch/b.c | Adds a simple C source using PCH with a pragma and header inclusion. |
| cpp/ql/integration-tests/header-variant-tests/microsoft-pch/a.c | Includes the corresponding header for testing purposes. |
| cpp/ql/integration-tests/header-variant-tests/clang-pch/test.py | Introduces a Python script to invoke database creation with PCH commands. |
| cpp/ql/integration-tests/header-variant-tests/clang-pch/*.c | Provides minimal C source files exercising different PCH inclusion scenarios. |
Comments suppressed due to low confidence (1)
cpp/ql/integration-tests/header-variant-tests/clang-pch/c.c:2
- The macro ONE is used but its definition isn't shown in the diff; ensure that it is defined or included appropriately in the test environment.
return ONE + FOUR;
| def test(codeql, cpp): | ||
| os.mkdir("pch") | ||
| codeql.database.create(command=[ | ||
| f'"{cpp.get_tool("extractor")}" --mimic-clang -emit-pch -o pch/a.pch a.c', |
There was a problem hiding this comment.
minor cosmetic nit: here you could define once extractor = f"{cpp.get_tool("extractor")} --mimic-clang" and then reuse that for all commands.
This also avoids an extremely minor FS churn with calling get_tool repeatedly (that tries tools/extractor first with a couple of file extensions, and then goes into tools/{CODEQL_PLAT}/extractor, again potentially with multiple file extensions)
There was a problem hiding this comment.
Fixed in edd18dc. I left the duplication of --mimic-clang/--mimic-cl, as I feel that slightly better signals what is going on.
These tests currently test a code path in the extractor that only exists for these tests. By turning them into integration tests, we actually use the code path that normal database creation uses.
Moreover, the tests only work by virtue of a hack, where we know magically that the empty file
_.cwill be extracted first by qltest.