Refactor old and create basic unit tests to important classes #51
Conversation
Signed-off-by: lbgracioso <contact@lbgracioso.net>
# Conflicts: # src/answerfile.cpp
Signed-off-by: lbgracioso <contact@lbgracioso.net>
Signed-off-by: lbgracioso <contact@lbgracioso.net>
Signed-off-by: lbgracioso <contact@lbgracioso.net>
Signed-off-by: lbgracioso <contact@lbgracioso.net>
Signed-off-by: lbgracioso <contact@lbgracioso.net>
Signed-off-by: lbgracioso <contact@lbgracioso.net>
Signed-off-by: lbgracioso <contact@lbgracioso.net>
…sses Signed-off-by: lbgracioso <contact@lbgracioso.net>
Signed-off-by: lbgracioso <contact@lbgracioso.net>
Signed-off-by: lbgracioso <contact@lbgracioso.net>
…ification.cpp tests Signed-off-by: lbgracioso <contact@lbgracioso.net>
Signed-off-by: lbgracioso <contact@lbgracioso.net>
Signed-off-by: lbgracioso <contact@lbgracioso.net>
Signed-off-by: lbgracioso <contact@lbgracioso.net>
|
@lbgracioso did you get the errors on SonarCloud? |
# Conflicts: # include/cloysterhpc/tests.h # src/answerfile.cpp # src/inifile.cpp
Signed-off-by: lbgracioso <contact@lbgracioso.net>
|
There was a problem hiding this comment.
@lbgracioso can you take a look on the comments?
I think there's a misconception of what a test should be. Some tests are runtime tests, and not effectively unit tests, that should be checked on runtime. It's integral part of application behaviour.
Some tests should be mocked and not interfere with the OS, the idea is to test the methods, classes and functions in a controlled space. Removing runtime tests or checks, does not add quality to the code but decreases it.
Remember that those tests are a developer feature and you must consider the user.
There is a reasonable talk here: https://www.youtube.com/watch?v=SAM4rWaIvUQ
Tips: https://stackify.com/unit-testing-basics-best-practices/
| #ifdef BUILD_TESTING | ||
| #include <cloysterhpc/tests.h> | ||
|
|
||
| TEST_SUITE("Test user privileges") |
There was a problem hiding this comment.
I don't get this one. This should be a runtime check and not effectively a test. Because it doesn't matter if it's compiled as root or not, but execution should be done with root.
| SUBCASE("Invalid Hostnames") | ||
| { | ||
| CHECK_THROWS(server.setHostname( | ||
| std::string(100, 'a'))); // Exceeds the maximum length |
There was a problem hiding this comment.
Usually should test against the max defined number, that I think it's 64. A good idea is to define a constexpr auto value and use the same on the class and on the tests.
| #ifdef BUILD_TESTING | ||
| #include <cloysterhpc/tests.h> | ||
|
|
||
| TEST_SUITE("Test OS class") |
There was a problem hiding this comment.
I didn't get this test also. What's testing exactly? Because those values are get during runtime and now during compile time.
| { | ||
|
|
||
| Network testNetwork; | ||
| auto testInterface = "enp0s25"; |
There was a problem hiding this comment.
Does this is a mock or just a fixed interface name from the OS you're running? If we change this to enp1s0f0d0 for instance, it will continue to pass the tests? If not we have a problem here.
| } | ||
| } | ||
|
|
||
| TEST_CASE("Add a string to file with cloyster::addStringToFile") |
There was a problem hiding this comment.
Honest question, tests that create files or modify them: I think this should be mocked, no?
| @@ -12,24 +12,10 @@ | |||
|
|
|||
| const std::filesystem::path& DiskImage::getPath() const { return m_path; } | |||
|
|
|||
| void DiskImage::setPath(const std::filesystem::path& path) | |||
Warning
Please ensure PR #50 is reviewed and merged before proceeding with this one. This PR is based on the changes in PR #50 and may include additional modifications (it may also include changes beyond the scope of the original PR). Until PR #50 is merged, this PR will remain as a draft.
This PR focuses on adding unit tests to almost all "very important classes".
While the PR may seem extensive, it consists of basic unit tests to validate specific functionalities. Although the PR appears huge, its scope is relatively small.