update test workflow and unittests files #24
Conversation
WalkthroughThe changes introduce a comprehensive testing setup for the project. A new Makefile and two C++ test files are added under the Changes
Sequence Diagram(s)sequenceDiagram
participant GitHub Actions
participant Makefile
participant test_trey_app
participant test_db_clipboard
GitHub Actions->>Makefile: make clean && make all
Makefile->>test_trey_app: Build executable
Makefile->>test_db_clipboard: Build executable
GitHub Actions->>test_trey_app: Run tray app unit tests
test_trey_app->>test_trey_app: Execute test_load_history(), test_gtk_init()
GitHub Actions->>test_db_clipboard: Run db/clipboard unit tests
test_db_clipboard->>test_db_clipboard: Execute test_append_and_load_history(), test_get_clipboard()
GitHub Actions->>GitHub Actions: Run E2E test script
Possibly related PRs
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
test/Makefile (1)
7-8: Consider consolidating library flags.The GTK and AppIndicator flags are repeated in the target-specific rule. Consider defining them as variables for better maintainability.
+GTK_FLAGS = `pkg-config --cflags --libs gtk+-3.0 appindicator3-0.1` + test_trey_app: test_trey_app.cpp ../src/db.cpp ../src/clipboard_mgmt.cpp ../src/logging.cpp - $(CXX) $(CXXFLAGS) $^ `pkg-config --cflags --libs gtk+-3.0 appindicator3-0.1` -lX11 -o $@ + $(CXX) $(CXXFLAGS) $^ $(GTK_FLAGS) $(LDFLAGS) -o $@.github/workflows/test.yml (2)
46-47: Fix trailing spaces.There are trailing spaces at the end of line 47.
- - name: Run hcp E2E test - run: xvfb-run --auto-servernum --server-args='-screen 0 1024x768x24' ./test_hcp.sh + - name: Run hcp E2E test + run: xvfb-run --auto-servernum --server-args='-screen 0 1024x768x24' ./test_hcp.sh
49-59: Fix trailing spaces and consider adding error handling.There are trailing spaces at the end of line 59. Also consider adding error handling for the test execution steps.
- name: Run tray app Unit test run: ./test/test_trey_app - name: Run db/clipboard Unit test - run: ./test/test_db_clipboard + run: ./test/test_db_clipboardConsider adding failure handling to ensure test failures are properly reported:
- name: Run tray app Unit test - run: ./test/test_trey_app + run: ./test/test_trey_app || exit 1 - name: Run db/clipboard Unit test - run: ./test/test_db_clipboard + run: ./test/test_db_clipboard || exit 1test/test_trey_app.cpp (2)
6-15: Enhance test validation beyond exception handling.The test only validates that no exceptions are thrown, but doesn't verify the actual functionality. Consider adding more specific assertions about the loaded data.
void test_load_history() { try { auto history = load_clipboard_blocks(); - std::cout << "[PASS] test_load_history -> Loaded " << history.size() << " clipboard entries.\n"; + std::cout << "[PASS] test_load_history -> Loaded " << history.size() << " clipboard entries.\n"; + + // Basic validation of loaded entries + for (const auto& entry : history) { + if (entry.empty()) { + std::cout << "[WARN] test_load_history -> Found empty clipboard entry.\n"; + } + } } catch (const std::exception& e) { std::cout << "[FAIL] test_load_history -> Exception: " << e.what() << "\n"; } catch (...) { std::cout << "[ERROR] test_load_history -> Unknown error\n"; } }
21-32: Consider adding GTK cleanup and more robust initialization.The GTK initialization test doesn't clean up after itself and may not work properly in headless environments.
void test_gtk_init() { try { int argc = 0; char **argv = nullptr; - gtk_init(&argc, &argv); + + // Set up for headless operation + g_setenv("DISPLAY", ":99", TRUE); + + if (gtk_init_check(&argc, &argv)) { + std::cout << "[PASS] test_gtk_init -> GTK initialized successfully.\n"; + } else { + std::cout << "[WARN] test_gtk_init -> GTK initialization failed (headless environment).\n"; + } std::cout << "[PASS] test_gtk_init -> GTK initialized successfully.\n"; } catch (const std::exception& e) { std::cout << "[FAIL] test_gtk_init -> Exception: " << e.what() << "\n"; } catch (...) { std::cout << "[ERROR] test_gtk_init -> Unknown error\n"; } }test/test_db_clipboard.cpp (2)
9-28: Enhance test validation and consider edge cases.The test validates basic functionality but could be more thorough. Based on the retrieved learning about bounds checking, consider testing edge cases for clipboard entry sizes.
void test_append_and_load_history() { try { std::string test_entry = "Test clipboard entry for db test"; append_clipboard_block(test_entry); auto history = load_clipboard_blocks(); bool found = false; for (const auto& entry : history) { if (entry == test_entry) found = true; } if (found) { std::cout << "[PASS] test_append_and_load_history -> Test entry found in clipboard history.\n"; } else { std::cout << "[FAIL] test_append_and_load_history -> Test entry NOT found in clipboard history.\n"; } + + // Test edge cases based on system clipboard limits + std::string large_entry(10000, 'X'); // Test large entry + try { + append_clipboard_block(large_entry); + std::cout << "[PASS] test_append_and_load_history -> Large entry handled correctly.\n"; + } catch (const std::exception& e) { + std::cout << "[INFO] test_append_and_load_history -> Large entry rejected: " << e.what() << "\n"; + } } catch (const std::exception& e) { std::cout << "[FAIL] test_append_and_load_history -> Exception: " << e.what() << "\n"; } catch (...) { std::cout << "[ERROR] test_append_and_load_history -> Unknown error\n"; } }
38-40: Use dynamic truncation for clipboard content display.The hardcoded 40-character truncation may not be appropriate for all clipboard content types or sizes.
- std::string clip = get_clipboard(display); - std::cout << "[PASS] test_get_clipboard -> Clipboard content: '" << clip.substr(0, 40) << "'...\n"; + std::string clip = get_clipboard(display); + size_t preview_length = std::min(clip.length(), size_t(40)); + std::string preview = clip.substr(0, preview_length); + + // Replace non-printable characters for better display + std::replace_if(preview.begin(), preview.end(), + [](char c) { return !std::isprint(c); }, '?'); + + std::cout << "[PASS] test_get_clipboard -> Clipboard content (" << clip.length() + << " chars): '" << preview; + if (clip.length() > preview_length) std::cout << "..."; + std::cout << "'\n";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/test.yml(1 hunks)test/Makefile(1 hunks)test/test_db_clipboard.cpp(1 hunks)test/test_trey_app.cpp(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
test/test_db_clipboard.cpp (1)
Learnt from: Agent-Hellboy
PR: Agent-Hellboy/hcp#2
File: src/db.cpp:30-36
Timestamp: 2025-07-03T19:24:22.002Z
Learning: In the hcp clipboard history project, bounds checking for clipboard entry sizes should be based on the system's clipboard limit rather than arbitrary size limits.
🪛 YAMLlint (1.37.1)
.github/workflows/test.yml
[error] 47-47: trailing spaces
(trailing-spaces)
[error] 59-59: trailing spaces
(trailing-spaces)
🪛 checkmake (0.2.2)
test/Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
[warning] 5-5: Target "all" should be declared PHONY.
(phonydeclared)
| CXX = g++ | ||
| CXXFLAGS = -std=c++11 -I../include | ||
| LDFLAGS = -lX11 | ||
|
|
||
| all: test_trey_app test_db_clipboard | ||
|
|
||
| test_trey_app: test_trey_app.cpp ../src/db.cpp ../src/clipboard_mgmt.cpp ../src/logging.cpp | ||
| $(CXX) $(CXXFLAGS) $^ `pkg-config --cflags --libs gtk+-3.0 appindicator3-0.1` -lX11 -o $@ | ||
|
|
||
| test_db_clipboard: test_db_clipboard.cpp ../src/db.cpp ../src/clipboard_mgmt.cpp ../src/logging.cpp | ||
| $(CXX) $(CXXFLAGS) $^ -lX11 -o $@ | ||
|
|
||
| clean: | ||
| rm -f test_trey_app test_db_clipboard No newline at end of file |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add phony target declarations to prevent conflicts.
The Makefile is missing .PHONY declarations for targets that don't create files with the same name. This could cause issues if files named all, clean, or test exist in the directory.
+.PHONY: all clean test
+
CXX = g++
CXXFLAGS = -std=c++11 -I../include
LDFLAGS = -lX11
all: test_trey_app test_db_clipboardConsider adding a test target that runs both executables:
+test: test_trey_app test_db_clipboard
+ ./test_trey_app
+ ./test_db_clipboard
+
clean:
rm -f test_trey_app test_db_clipboard🧰 Tools
🪛 checkmake (0.2.2)
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
[warning] 5-5: Target "all" should be declared PHONY.
(phonydeclared)
🤖 Prompt for AI Agents
In test/Makefile lines 1 to 14, add a .PHONY declaration for the targets all,
clean, test_trey_app, and test_db_clipboard to prevent conflicts with files of
the same name. Also, add a new test target that depends on both test_trey_app
and test_db_clipboard and runs them sequentially. This ensures proper execution
and avoids issues with file name conflicts.
| Display* display = XOpenDisplay(nullptr); | ||
| if (!display) { | ||
| std::cout << "[ERROR] test_get_clipboard -> X11 display not available, skipping clipboard test.\n"; | ||
| return; | ||
| } | ||
| std::string clip = get_clipboard(display); | ||
| std::cout << "[PASS] test_get_clipboard -> Clipboard content: '" << clip.substr(0, 40) << "'...\n"; | ||
| XCloseDisplay(display); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider resource cleanup in error cases.
The X11 display is only closed on successful execution. If an exception occurs, the display handle may leak.
void test_get_clipboard() {
+ Display* display = nullptr;
try {
- Display* display = XOpenDisplay(nullptr);
+ display = XOpenDisplay(nullptr);
if (!display) {
std::cout << "[ERROR] test_get_clipboard -> X11 display not available, skipping clipboard test.\n";
return;
}
std::string clip = get_clipboard(display);
std::cout << "[PASS] test_get_clipboard -> Clipboard content: '" << clip.substr(0, 40) << "'...\n";
- XCloseDisplay(display);
} catch (const std::exception& e) {
std::cout << "[FAIL] test_get_clipboard -> Exception: " << e.what() << "\n";
} catch (...) {
std::cout << "[ERROR] test_get_clipboard -> Unknown error\n";
}
+
+ if (display) {
+ XCloseDisplay(display);
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Display* display = XOpenDisplay(nullptr); | |
| if (!display) { | |
| std::cout << "[ERROR] test_get_clipboard -> X11 display not available, skipping clipboard test.\n"; | |
| return; | |
| } | |
| std::string clip = get_clipboard(display); | |
| std::cout << "[PASS] test_get_clipboard -> Clipboard content: '" << clip.substr(0, 40) << "'...\n"; | |
| XCloseDisplay(display); | |
| void test_get_clipboard() { | |
| Display* display = nullptr; | |
| try { | |
| display = XOpenDisplay(nullptr); | |
| if (!display) { | |
| std::cout << "[ERROR] test_get_clipboard -> X11 display not available, skipping clipboard test.\n"; | |
| return; | |
| } | |
| std::string clip = get_clipboard(display); | |
| std::cout << "[PASS] test_get_clipboard -> Clipboard content: '" << clip.substr(0, 40) << "'...\n"; | |
| } catch (const std::exception& e) { | |
| std::cout << "[FAIL] test_get_clipboard -> Exception: " << e.what() << "\n"; | |
| } catch (...) { | |
| std::cout << "[ERROR] test_get_clipboard -> Unknown error\n"; | |
| } | |
| if (display) { | |
| XCloseDisplay(display); | |
| } | |
| } |
🤖 Prompt for AI Agents
In test/test_db_clipboard.cpp around lines 33 to 40, the X11 display opened by
XOpenDisplay is only closed on successful execution, risking resource leaks if
an exception occurs. To fix this, ensure that XCloseDisplay is called regardless
of success or failure by using a try-finally pattern or a RAII wrapper to
guarantee the display is closed even if an exception is thrown or the function
returns early.
There was a problem hiding this comment.
Add a comment inside #25 , I will use that ticket to capture resource leaks.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First check if issue #25 exists
gh issue view 25 --json title,body,url
# Add comment to issue #25 about the resource leak
gh issue comment 25 --body "## Resource leak in test_db_clipboard.cpp
Found during review of PR #24: https://github.com/Agent-Hellboy/hcp/pull/24#discussion_r2193126212
**Issue:** In the \`test_get_clipboard()\` function, the X11 display opened by \`XOpenDisplay()\` is only closed on successful execution, risking resource leaks if an exception occurs.
**Location:** \`test/test_db_clipboard.cpp\` lines 33-40
**Details:** The current code closes the display handle only when the function completes successfully. If an exception is thrown by \`get_clipboard()\`, the display resource may leak.
**Suggested Fix:** Ensure that \`XCloseDisplay()\` is called regardless of success or failure by using proper exception handling or RAII pattern.
Reported by: @Agent-Hellboy"
Length of output: 1330
@Agent-Hellboy I've successfully added a comment to issue #25 documenting the resource leak found in the test_get_clipboard() function. The comment includes:
- Reference to the current PR and review discussion
- Detailed description of the resource leak issue
- Location of the problematic code
- Suggested fix approach
The comment has been posted at: #25 (comment)
This will help track the resource leak for future resolution while keeping the current PR focused on the test workflow updates.
🧠 Learnings used
Learnt from: Agent-Hellboy
PR: Agent-Hellboy/hcp#2
File: src/db.cpp:30-36
Timestamp: 2025-07-03T19:24:22.047Z
Learning: In the hcp clipboard history project, bounds checking for clipboard entry sizes should be based on the system's clipboard limit rather than arbitrary size limits.
Summary by CodeRabbit
New Features
Chores