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

Improve the test coverage of the C API #447

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Gnimuc
Copy link
Contributor

@Gnimuc Gnimuc commented Jan 13, 2025

Description

as per #440 (comment)

Fixes # (issue)

Type of change

Please tick all options which are relevant.

  • Bug fix
  • New feature
  • Requires documentation updates

Testing

Please describe the test(s) that you added and ran to verify your changes.

Checklist

  • I have read the contribution guide recently

@Gnimuc Gnimuc changed the title Improve the test coverage of the C API -- Take 1 Improve the test coverage of the C API Jan 13, 2025
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

unittests/CppInterOp/InterpreterTest.cpp Outdated Show resolved Hide resolved
unittests/CppInterOp/InterpreterTest.cpp Outdated Show resolved Hide resolved
@Gnimuc Gnimuc marked this pull request as ready for review January 13, 2025 10:16
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

unittests/CppInterOp/FunctionReflectionTest.cpp Outdated Show resolved Hide resolved
unittests/CppInterOp/FunctionReflectionTest.cpp Outdated Show resolved Hide resolved
unittests/CppInterOp/FunctionReflectionTest.cpp Outdated Show resolved Hide resolved
unittests/CppInterOp/FunctionReflectionTest.cpp Outdated Show resolved Hide resolved
unittests/CppInterOp/FunctionReflectionTest.cpp Outdated Show resolved Hide resolved
unittests/CppInterOp/InterpreterTest.cpp Show resolved Hide resolved
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

lib/Interpreter/CXCppInterOp.cpp Outdated Show resolved Hide resolved
unittests/CppInterOp/ScopeReflectionTest.cpp Show resolved Hide resolved
unittests/CppInterOp/ScopeReflectionTest.cpp Show resolved Hide resolved
unittests/CppInterOp/TypeReflectionTest.cpp Show resolved Hide resolved
@Gnimuc Gnimuc force-pushed the c-api-test branch 3 times, most recently from 6f81874 to 11fea7d Compare January 13, 2025 11:57
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

EXPECT_TRUE(Cpp::Process("") == 0);
EXPECT_TRUE(Cpp::Process("int a = 12;") == 0);
EXPECT_FALSE(Cpp::Process("error_here;") == 0);
// Linker/JIT error.
EXPECT_FALSE(Cpp::Process("int f(); int res = f();") == 0);

// C API
auto CXI = clang_createInterpreterFromRawPtr(I);
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: 'auto CXI' can be declared as 'auto *CXI' [llvm-qualified-auto]

Suggested change
auto CXI = clang_createInterpreterFromRawPtr(I);
auto *CXI = clang_createInterpreterFromRawPtr(I);

Copy link
Contributor

Choose a reason for hiding this comment

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

This seems a good suggestion.

Copy link

codecov bot commented Jan 13, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 74.92%. Comparing base (54c9482) to head (b7c0766).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #447      +/-   ##
==========================================
+ Coverage   71.15%   74.92%   +3.76%     
==========================================
  Files           9        9              
  Lines        3550     3549       -1     
==========================================
+ Hits         2526     2659     +133     
+ Misses       1024      890     -134     
Files with missing lines Coverage Δ
lib/Interpreter/CXCppInterOp.cpp 49.28% <100.00%> (+27.18%) ⬆️
lib/Interpreter/CppInterOp.cpp 80.46% <100.00%> (+0.03%) ⬆️

... and 2 files with indirect coverage changes

Files with missing lines Coverage Δ
lib/Interpreter/CXCppInterOp.cpp 49.28% <100.00%> (+27.18%) ⬆️
lib/Interpreter/CppInterOp.cpp 80.46% <100.00%> (+0.03%) ⬆️

... and 2 files with indirect coverage changes


// C API
auto* I = clang_createInterpreterFromRawPtr(Cpp::GetInterpreter());
EXPECT_TRUE(clang_hasDefaultConstructor(make_scope(Decls[0], I)));
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think we should put the tests for the c layer in a separate file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think that's practical at the moment because of the low API coverage. We need to use a lot C++ API to set up the environment and do the test.

@Gnimuc Gnimuc force-pushed the c-api-test branch 2 times, most recently from 2a1eca2 to 0420a80 Compare January 18, 2025 06:03
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

output = testing::internal::GetCapturedStdout();
EXPECT_EQ(output, "Constructor Executed");
output.clear();
auto dummy = clang_allocate(8);
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: 'auto dummy' can be declared as 'auto *dummy' [llvm-qualified-auto]

Suggested change
auto dummy = clang_allocate(8);
auto *dummy = clang_allocate(8);

@Gnimuc
Copy link
Contributor Author

Gnimuc commented Jan 18, 2025

This only improved the coverage from 70% to 74% because as I said before nearly half of the code are from libclang.

@Gnimuc Gnimuc requested a review from vgvassilev January 18, 2025 11:29
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

@@ -15,6 +15,8 @@

#include "llvm/Support/Valgrind.h"

#include "clang-c/CXCppInterOp.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: duplicate include [readability-duplicate-include]

unittests/CppInterOp/ScopeReflectionTest.cpp:16:

- 
- #include "clang-c/CXCppInterOp.h"

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.

2 participants