[ISSUE-812] Use an LLM to aid in generating basic unit tests for all classes#909
[ISSUE-812] Use an LLM to aid in generating basic unit tests for all classes#909kblricks wants to merge 2 commits intoSharedDevelopmentfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds two instructional markdown files to help GitHub Copilot assist with unit test generation and code reviews in the Graphitti project.
Changes:
- Adds
.github/prompts/generate-unit-tests.prompt.mdwith comprehensive guidance for generating Google Test unit tests for C++ code - Adds
.github/copilot-instructions.mdwith project-specific coding standards, conventions, and PR review guidelines
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
.github/prompts/generate-unit-tests.prompt.md |
Provides detailed instructions for LLM-assisted unit test generation, including Google Test basics, test structure requirements, assertion references, and guidelines for analyzing existing tests |
.github/copilot-instructions.md |
Documents Graphitti project overview, C++17 coding standards, formatting conventions, and comprehensive PR review checklist to guide Copilot in code reviews |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| class {ClassName}Test : public ::testing::Test { | ||
| protected: | ||
| void SetUp() override { | ||
| // Initialize test objects before each test | ||
| } | ||
|
|
||
| void TearDown() override { | ||
| // Clean up resources after each test | ||
| } | ||
|
|
||
| // Shared test data members | ||
| }; | ||
|
|
||
| // Standalone test example | ||
| TEST({ClassName}Test, MethodName_ValidInput_ReturnsExpected) { | ||
| // Arrange - set up test data | ||
|
|
||
| // Act - call the function under test | ||
|
|
||
| // Assert - verify the results | ||
| } | ||
|
|
||
| // Fixture-based test example | ||
| TEST_F({ClassName}Test, MethodName_EdgeCase_HandlesCorrectly) { |
There was a problem hiding this comment.
The template shows inconsistent naming for test fixtures and test suites. In the actual codebase, test fixtures use a "Fixture" suffix (e.g., InputManagerFixture), while standalone TEST() macros use the class name without a suffix (e.g., TEST(ConnectionsFactory, ...)). The template should be updated to reflect this convention:
- For fixtures: class {ClassName}Fixture : public ::testing::Test
- For standalone tests: TEST({ClassName}, ...)
- For fixture-based tests: TEST_F({ClassName}Fixture, ...)
| x++; | ||
| } | ||
|
|
||
| int f(a) |
There was a problem hiding this comment.
The code example shows an invalid C++ function signature int f(a) where parameter a is missing a type. This should be corrected to something like int f(int a) to be valid C++. While this error appears to have been copied from docs/Developer/codingConventions.md:35, it should be fixed in this new file.
| int f(a) | |
| int f(int a) |
| ```cpp | ||
| class A { | ||
| const int i = 100; | ||
| const int * const num = &i; | ||
|
|
||
| int function1(char c) const; | ||
| const int* function2(const char* string) const; | ||
| }; | ||
| ``` | ||
|
|
||
| ### Copy and Move Operations | ||
|
|
||
| - A class must make it clear whether it is copyable, move-only, or neither by explicitly declaring and/or deleting the appropriate operations | ||
| - Use compiler options `= default` and `= delete` | ||
|
|
||
| ```cpp | ||
| class Copyable { | ||
| public: | ||
| Copyable(const Copyable& other) = default; | ||
| Copyable& operator=(const Copyable& other) = default; | ||
| }; | ||
|
|
||
| class MoveOnly { | ||
| public: | ||
| MoveOnly(MoveOnly&& other) = default; | ||
| MoveOnly& operator=(MoveOnly&& other) = default; | ||
| }; | ||
|
|
||
| class NotCopyable { | ||
| public: | ||
| NotCopyable(const NotCopyable&) = delete; | ||
| NotCopyable& operator=(const NotCopyable&) = delete; | ||
| }; |
There was a problem hiding this comment.
The code examples use inconsistent indentation (appears to be 1-2 spaces) which doesn't match the project's 3-space indentation standard. While this was copied from docs/Developer/cppStyleGuide.md, it should be corrected in this new file to use 3 spaces for consistency with the actual codebase (e.g., see Simulator/Core/Simulator.h:28-32).
| ```cpp | ||
| if (x > m) { | ||
| x--; | ||
| } else { | ||
| x++; | ||
| } | ||
|
|
||
| int f(a) | ||
| { | ||
| return a; | ||
| } |
There was a problem hiding this comment.
The code example appears to use 4-space indentation instead of the project's 3-space standard. This should be corrected to match the project convention documented on line 43.
|
|
||
| - **Language**: C++17 | ||
| - **Build System**: CMake | ||
| - **Testing**: Custom testing framework with unit and regression tests |
There was a problem hiding this comment.
The description states "Custom testing framework with unit and regression tests" but this is incorrect. The project uses Google Test (gtest) for unit testing, as evidenced by the GoogleTestsTutorial.md documentation and all test files in Testing/UnitTesting/. This should be updated to "Google Test (gtest) for unit testing and custom regression tests" or similar.
| - **Testing**: Custom testing framework with unit and regression tests | |
| - **Testing**: Google Test (gtest) for unit testing and custom regression tests |
1 participant
Closes #812
Description
Adds a markdown file for instructing Copilot to generate test cases, and a markdown file for instructing Copilot to use relevant context when reviewing PRs
Checklist (Mandatory for new features)
Testing (Mandatory for all changes)
test-medium-connected.xmlPassedtest-large-long.xmlPassed