🧪 [testing improvement] Add unit tests for MediaFile::split edge cases#287
🧪 [testing improvement] Add unit tests for MediaFile::split edge cases#287
Conversation
This change introduces comprehensive unit tests for both variants of `MediaFile::split`. The tests cover standard comma separation, empty strings, consecutive delimiters, and the known std::getline quirk with trailing delimiters. The Makefile.am has also been updated to integrate the new test into the `make check` suite. Co-authored-by: segin <480709+segin@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
There was a problem hiding this comment.
Pull request overview
Adds targeted unit test coverage for the MediaFile::split string tokenization helpers to prevent regressions across common and edge-case delimiter scenarios.
Changes:
- Added a new C++ unit test executable covering
MediaFile::splitbehaviors (empty input, missing delimiter, consecutive delimiters, trailing delimiter quirk, and append-variant). - Wired the new test into the Automake test build/run configuration.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
tests/test_mediafile_split.cpp |
New unit tests validating MediaFile::split edge cases and both overload variants. |
tests/Makefile.am |
Registers test_mediafile_split in the test build/run targets. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| check_PROGRAMS += test_mediafile_split | ||
| test_mediafile_split_SOURCES = test_mediafile_split.cpp | ||
| test_mediafile_split_LDADD = $(COMMON_TEST_LIBS) $(AM_LDFLAGS) | ||
| TESTS += test_mediafile_split |
There was a problem hiding this comment.
TESTS is already set to $(check_PROGRAMS) earlier in this Makefile, so appending test_mediafile_split to TESTS will cause this executable to be run twice under make check. Drop the TESTS += test_mediafile_split line (or, if this test is meant to run without being in check_PROGRAMS, remove the check_PROGRAMS += ... instead).
| TESTS += test_mediafile_split |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces unit tests for the MediaFile::split function by adding a new test file, tests/test_mediafile_split.cpp, and updating tests/Makefile.am to include it in the build process. A redundancy was identified in the Makefile where the new test is added to both check_PROGRAMS and TESTS, which would result in the test being executed twice during the check phase.
| check_PROGRAMS += test_mediafile_split | ||
| test_mediafile_split_SOURCES = test_mediafile_split.cpp | ||
| test_mediafile_split_LDADD = $(COMMON_TEST_LIBS) $(AM_LDFLAGS) | ||
| TESTS += test_mediafile_split |
There was a problem hiding this comment.
The explicit addition of test_mediafile_split to TESTS at line 3856 is redundant. In this Makefile.am, TESTS is already defined as $(check_PROGRAMS) at line 3815. Since test_mediafile_split is added to check_PROGRAMS at line 3853, it will be automatically included in the test suite. Listing it again in TESTS will cause the test to be executed twice during make check.
check_PROGRAMS += test_mediafile_split
test_mediafile_split_SOURCES = test_mediafile_split.cpp
test_mediafile_split_LDADD = $(COMMON_TEST_LIBS) $(AM_LDFLAGS)
🎯 What: The testing gap addressed is the lack of unit tests for the simple, pure
MediaFile::splitutility functions.📊 Coverage: The new tests in
tests/test_mediafile_split.cppcover basic splitting, empty strings, no delimiter present, consecutive delimiters, and the known edge case of trailing delimiters not generating an empty string. Both the new-vector and append-vector variants are covered.✨ Result: Increased test coverage and guaranteed robustness of string tokenization across the codebase, preventing future regressions.
PR created automatically by Jules for task 6764116392174667634 started by @segin