🧪 Exhaustive tests for MediaFile::split edge cases

[segin]

🎯 What: The testing gap addressed was the lack of unit tests for the MediaFile::split function, which is a fundamental string utility used in the project.

📊 Coverage: The new test suite covers 9 distinct scenarios:

1. Standard comma-separated strings (multi-element).
2. Empty input strings (0 elements).
3. Strings without the delimiter (1 element).
4. Leading delimiters (empty first element).
5. Trailing delimiters (characterizing std::getline behavior).
6. Consecutive delimiters (empty elements in between).
7. Only delimiters (N elements for N delimiters at start/middle).
8. Different delimiter characters (e.g., '/').
9. Appending to an existing vector (preserving original elements).

✨ Result: Significant improvement in the test coverage for core utilities. The tests ensure that future refactoring of string processing logic can be done with confidence, and the behavior of edge cases is explicitly documented and verified.

---

PR created automatically by Jules for task 16080039616014497639 started by @segin

[google-labs-jules]

👋 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 @jules. You can find this option in the Pull Request section of your global Jules UI settings. You can always switch back!

New to Jules? Learn more at jules.google/docs.

---

For security, I will only act on instructions from the user who triggered this task.

[gemini-code-assist]

Code Review

This pull request introduces a new unit test suite for the MediaFile::split function, covering various edge cases such as empty strings, leading/trailing delimiters, and consecutive delimiters. The review feedback identifies that the current implementation of MediaFile::split is lossy when handling trailing delimiters, leading to inconsistent results compared to leading or middle delimiters. It is recommended to fix the underlying implementation to ensure trailing delimiters produce empty string elements. Additionally, the manual definition of the FINAL_BUILD macro in the test file should be moved to the build system to prevent potential inconsistencies between the test and production environments.

[gemini-code-assist]

This test case confirms an inconsistent behavior in MediaFile::split. While leading and middle delimiters correctly produce empty strings (as seen in Test 4 and Test 6), a trailing delimiter is ignored, making the split operation lossy. For example, "a," should ideally result in ["a", ""] to be consistent with ",a" resulting in ["", "a"]. The current behavior is a side effect of using std::getline without handling the trailing delimiter case.

I recommend fixing the implementation in src/mediafile.cpp to handle trailing delimiters and then updating this test to reflect the correct behavior. A possible fix for the implementation:

std::vector<std::string> &MediaFile::split(const std::string &s, char delim, std::vector<std::string> &elems) {
    if (s.empty()) return elems;
    std::stringstream ss(s);
    std::string item;
    while(std::getline(ss, item, delim)) {
        elems.push_back(item);
    }
    if (s.back() == delim) {
        elems.push_back("");
    }
    return elems;
}

            ASSERT_EQUALS(static_cast<size_t>(2), result.size(), "Trailing delimiter should result in an empty last element");
            if (result.size() >= 2) {
                ASSERT_EQUALS(std::string("trailing"), result[0], "First element mismatch");
                ASSERT_EQUALS(std::string(""), result[1], "Last element should be empty");
            }

[gemini-code-assist]

Similar to Test 5, this test confirms that the split operation is lossy when multiple delimiters are present at the end of the string. For ",,", a standard split should return 3 elements: ["", "", ""]. The current implementation returns only 2, which is inconsistent with how middle delimiters are handled. This should be updated once the underlying implementation is fixed to handle trailing delimiters.

            ASSERT_EQUALS(static_cast<size_t>(3), result.size(), "N delimiters should return N+1 elements");
            if (result.size() >= 3) {
                ASSERT_EQUALS(std::string(""), result[0], "First element mismatch");
                ASSERT_EQUALS(std::string(""), result[1], "Second element mismatch");
                ASSERT_EQUALS(std::string(""), result[2], "Third element mismatch");
            }

[gemini-code-assist]

Manually defining FINAL_BUILD within a test source file is discouraged. This macro appears to be a global build configuration that should be controlled via the build system (e.g., compiler flags in Makefile.am or a configuration header). Hardcoding it here can lead to inconsistencies between the test environment and the actual library build, potentially masking bugs or causing ODR violations if linked with other components.

[Copilot]

Pull request overview

Adds a dedicated unit test executable to cover edge-case behavior of the MediaFile::split string utility, and wires it into the Automake test runner so it executes under make check.

Changes:

• Introduces tests/test_mediafile_split.cpp covering 9 split scenarios (empty input, missing/leading/trailing/consecutive delimiters, alternate delimiter, and append-to-vector overload).
• Registers the new test_mediafile_split executable in tests/Makefile.am so it’s built and run as part of the test suite.

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 exercising MediaFile::split edge cases and append semantics.
tests/Makefile.am	Adds test_mediafile_split to check_PROGRAMS and defines its build/link rules.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

This new unit test file is missing the standard project copyright/license header that most other unit tests include (e.g., tests/test_base64_unit.cpp / tests/test_rect_area_validation.cpp). Please add the usual ISC license boilerplate and copyright line for consistency and licensing compliance.

Suggested change

	* This file is part of PsyMP3.
	* This file is part of PsyMP3.
	*
	* Copyright (c) 2024 PsyMP3 contributors
	*
	* Permission to use, copy, modify, and/or distribute this software for any
	* purpose with or without fee is hereby granted, provided that the above
	* copyright notice and this permission notice appear in all copies.
	*
	* THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
	* WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
	* MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
	* ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
	* WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
	* ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
	* OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.