Persist partition functionality #2396
Conversation
MaxKellermann
left a comment
MaxKellermann
left a comment
There was a problem hiding this comment.
I stopped after a while. Sorry, but this looks like a LLM-generated mess.
| * manages their lifecycle, and provides an API for sending commands | ||
| * and verifying state changes. | ||
| */ | ||
|
|
There was a problem hiding this comment.
MPD's own includes should be first
| #include <iostream> | ||
| #include <fstream> |
There was a problem hiding this comment.
Do not use C++ iostreams, please. It's a horrible abomination.
| #include <fstream> | ||
| #include <string> | ||
| #include <vector> | ||
| #include <filesystem> |
There was a problem hiding this comment.
And do not use std::filesystem either. It's less horrible than iostreams, but bad enough that MPD has its own abstractions for the same thing.
| * | ||
| */ | ||
| class MpdTestFixture { | ||
| private: |
| // MPD process and connection state | ||
| pid_t mpd_pid_ = -1; | ||
| int sock_ = -1; | ||
| std::string mpd_executable_ = "mpd"; |
There was a problem hiding this comment.
Why is this a std::string? It is never even modified.
| * @param command The MPD protocol command to send | ||
| * @return The response from MPD, or error message if not connected | ||
| */ | ||
| [[nodiscard]] std::string SendCommand(const std::string& command) { |
There was a problem hiding this comment.
Don't pass std::string around if you don't need one.
| } | ||
|
|
||
| std::string cmd = command; | ||
| if (cmd.empty() || cmd.back() != '\n') { |
There was a problem hiding this comment.
If the command is empty, send a bare newline character? What's the point of that?
| return std::string(buffer); | ||
| } | ||
|
|
||
| return ""; |
There was a problem hiding this comment.
Please don't construct a std::string from an empty C string literal. Just return {} if you want an empty string. Cheaper.
| // Give filesystem time to sync state file | ||
| std::this_thread::sleep_for(std::chrono::milliseconds(shutdown_delay_ms)); |
There was a problem hiding this comment.
I don't understand that. What filesystem? How does it sync a file?
| } | ||
|
|
||
| /** | ||
| * Force kill the MPD process using SIGTERM. |
There was a problem hiding this comment.
What does "force" mean here?
How would one kill MPD "without force"? What signal is less forceful than SIGTERM?
|
Thanks for taking the time to comment. Based on your feedback I agree that it is a mess. I did not take the testing seriously enough that is clear. I will take another pass at seeing if I can figure out how to test directly against StateFile(). I assume that is the proper way of testing the changes. Otherwise, someone else may be in better position to work on a change like this. |
|
Adding unit tests is very valuable, and I welcome that you tried to have a unit test for new code, and a "big one" that really starts a MPD process is certainly also a very good thing. Unit test code doesn't need to be as perfect as actual production code, but what you have now isn't good and robust enough. A fragile unit test that fails half of the time (maybe due to timing problems) only annoys, but isn't helpful. Maybe for this one, we should figure out how to put the unit test at a lower level, to only check the actual state file code without launching a full MPD process. Of course, that has its own set of difficulties, because the state file code is intertwined with all subsystems that it affects. The |
|
I will explore the two options further. I expect it will take me a chunk of time so will close this pull request. Thanks again for your insight and for MPD. We use it every day here. |
2 participants
I ended up separating out the integration test framework and test cases into two separate commits.
The thought was that if it is decided the use the test framework (which is required for the test cases), then maybe the framework should be further split out from the test cases. The framework could be used by other functionality that would benefit from integration testing.
I am probably over thinking it so let me know.