hyprtester: minor refactoring/restructure (#14154)

* hyprtester: Minor misc fixes

- Use `std::filesystem::path` instead of `std::string` in some places;
- Use `std::ranges::sort`, as suggested by clang-tidy;
- Reset colors at the end of every log line;
- Give more specific error messages in main.

* hyprtester: Fix a bug in `hyprlandAlive()`

Successful standard library calls are permitted to (and do) modify the
value of `errno` arbitrarily. Checking the value of `errno` only makes
sense when the library function returns an error. So, the correct
condition of a successful signal delivery is that the call to `kill`
returns 0.

Btw, given the possible other error values of `kill(2)`, should we even
consider `errno` at all?

* hyprtester: Restructure main

Split main's `main()` into multiple functions, separate responsibilities
more cleanly.

Add default values for cmd-line options to the help message.

* hyprtester: Remove redundant path canonicalization

* hyprtester: Run unified clean up code before every test

* tests: Use RAII for clients in client tests

Turn clients in client tests into classes. Constructors
initialize, destructors deinitialize.

Not only is it better design when entities (clients) are
modeled as classes, keeping all the methods connected,
this also ensures the tests perform their custom cleanup.

This will be especially important when we introduce early
termination on failure into the tests.

* hyprtester: Rewrite the testing framework!

- Implement a new test definition system used via the `TEST_CASE` and
  `SUBTEST` macros.

- Adds `ASSERT*` and reimplements `EXPECT*` macros. The former now
  terminate the test immediately. Also add some new macros, such as
  `FAIL_TEST`.

- Move existing tests to the new system. For trivial cases, break
  existing test functions down to multiple test cases.

- For non-trivial cases and a few other possible improvements, leave
  TODOs.

* tests: Use `EXPECT*` instead of `ASSERT*` in some places

`ASSERT*` macros terminate the test as soon as they fail. They are
appropriate to use when, if something fails, the rest of the test
does not make sense and more error messages from failing checks will
just pollute the output.

`EXPECT*` macros mark the test as failed but its execution continues.
It makes sense to use them when error messages from the following checks
in the test may be useful for someone who will be reading the log.

I changed some uses of `ASSERT*` in main tests to `EXPECT*`. But I was
not reading tests very carefully, so maybe I was wrong in some places.
My rule of thumb was:
- Checks that ensure that a kitty opens / a certain set/number of
  windows exists are fatal;
- A series of checks, especially on the same string returned by
  `Hyprland`, are non-fatal;
- Checks in tests with tightly coupled steps (i.e., do this, check,
  do this with the result of the previous step, check, etc) are fatal.

1 files changed, 152 insertions, 129 deletions

diff --git a/hyprtester/src/main.cpp b/hyprtester/src/main.cpp
index 2fc78201..0da9a20e 100644
--- a/hyprtester/src/main.cpp
+++ b/hyprtester/src/main.cpp
@@ -17,14 +17,17 @@
 #include "shared.hpp"
 #include "hyprctlCompat.hpp"
 #include "tests/main/tests.hpp"
+#undef TEST_CASES_STORAGE // Prevent redefinition warning
 #include "tests/clients/tests.hpp"
+#undef TEST_CASES_STORAGE // Prevent redefinition warning
 #include "tests/plugin/plugin.hpp"
+#include "tests/shared.hpp"
 
+#include <algorithm>
 #include <filesystem>
 #include <hyprutils/os/Process.hpp>
 #include <hyprutils/memory/WeakPtr.hpp>
 #include <hyprutils/memory/Casts.hpp>
-using namespace Hyprutils::Memory;
 
 #include <csignal>
 #include <cerrno>
@@ -38,35 +41,13 @@ using namespace Hyprutils::Memory;
 
 using namespace Hyprutils::OS;
 using namespace Hyprutils::Memory;
+using Path = std::filesystem::path;
 
 #define SP CSharedPointer
 
-static int               ret = 0;
-static SP<CProcess>      hyprlandProc;
-static const std::string cwd = std::filesystem::current_path().string();
-
-//
-static bool launchHyprland(std::string configPath, std::string binaryPath) {
-    if (binaryPath == "") {
-        std::error_code ec;
-        if (!std::filesystem::exists(cwd + "/../build/Hyprland", ec) || ec) {
-            NLog::log("{}No Hyprland binary", Colors::RED);
-            return false;
-        }
-
-        binaryPath = cwd + "/../build/Hyprland";
-    }
-
-    if (configPath == "") {
-        std::error_code ec;
-        if (!std::filesystem::exists(cwd + "/test.lua", ec) || ec) {
-            NLog::log("{}No test config", Colors::RED);
-            return false;
-        }
-
-        configPath = cwd + "/test.lua";
-    }
+static SP<CProcess> hyprlandProc;
 
+static bool         launchHyprland(Path configPath, Path binaryPath) {
     NLog::log("{}Launching Hyprland", Colors::YELLOW);
     hyprlandProc = makeShared<CProcess>(binaryPath, std::vector<std::string>{"--config", configPath});
     hyprlandProc->addEnv("HYPRLAND_HEADLESS_ONLY", "1");
@@ -78,121 +59,147 @@ static bool launchHyprland(std::string configPath, std::string binaryPath) {
 
 static bool hyprlandAlive() {
     NLog::log("{}hyprlandAlive", Colors::YELLOW);
-    kill(hyprlandProc->pid(), 0);
-    return errno != ESRCH;
+    return kill(hyprlandProc->pid(), 0) == 0 || errno != ESRCH;
 }
 
-static void help() {
+[[noreturn]] static void helpAndDie(int exit_code) {
     NLog::log("usage: hyprtester [arg [...]].\n");
     NLog::log(R"(Arguments:
     --help              -h       - Show this message again
-    --config FILE       -c FILE  - Specify config file to use
-    --binary FILE       -b FILE  - Specify Hyprland binary to use
-    --plugin FILE       -p FILE  - Specify the location of the test plugin)");
-}
-
-int main(int argc, char** argv, char** envp) {
-
-    std::string configPath = "";
-    std::string binaryPath = "";
-    std::string pluginPath = std::filesystem::current_path().string();
-
-    if (argc > 1) {
-        std::span<char*> args{argv + 1, sc<std::size_t>(argc - 1)};
+    --config FILE       -c FILE  - Specify config file to use (default: './test.lua')
+    --binary FILE       -b FILE  - Specify Hyprland binary to use (default: '../build/Hyprland')
+    --plugin FILE       -p FILE  - Specify the location of the test plugin (default: './'))");
 
-        for (auto it = args.begin(); it != args.end(); it++) {
-            std::string_view value = *it;
-
-            if (value == "--config" || value == "-c") {
-                if (std::next(it) == args.end()) {
-                    help();
-
-                    return 1;
-                }
-
-                configPath = *std::next(it);
-
-                try {
-                    configPath = std::filesystem::canonical(configPath);
-
-                    if (!std::filesystem::is_regular_file(configPath)) {
-                        throw std::exception();
-                    }
-                } catch (...) {
-                    std::println(stderr, "[ ERROR ] Config file '{}' doesn't exist!", configPath);
-                    help();
-
-                    return 1;
-                }
-
-                it++;
+    std::exit(exit_code);
+}
 
-                continue;
-            } else if (value == "--binary" || value == "-b") {
-                if (std::next(it) == args.end()) {
-                    help();
+static Path validatePathOrDie(Path path) {
+    try {
+        if (!std::filesystem::is_regular_file(path)) {
+            throw std::exception();
+        }
+    } catch (...) {
+        std::println(stderr, "[ ERROR ] File '{}' is not accessible or not a regular file", path.string());
+        helpAndDie(EXIT_FAILURE);
+    }
+    return path;
+}
 
-                    return 1;
-                }
+namespace {
+    struct SSettings {
+        Path configPath;
+        Path binaryPath;
+        Path pluginPath;
+    };
+}
 
-                binaryPath = *std::next(it);
+static SSettings parseSettings(const std::span<const char*> args) {
+    static const auto cwd = std::filesystem::current_path();
+    SSettings         settings{};
 
-                try {
-                    binaryPath = std::filesystem::canonical(binaryPath);
+    for (auto it = args.begin(); it < args.end(); it++) {
+        std::string_view value = *it;
 
-                    if (!std::filesystem::is_regular_file(binaryPath)) {
-                        throw std::exception();
-                    }
-                } catch (...) {
-                    std::println(stderr, "[ ERROR ] Binary '{}' doesn't exist!", binaryPath);
-                    help();
+        if (value == "--config" || value == "-c") {
+            if (std::next(it) == args.end()) {
+                helpAndDie(EXIT_FAILURE);
+            }
 
-                    return 1;
-                }
+            settings.configPath = validatePathOrDie(*std::next(it));
+            it++;
+        } else if (value == "--binary" || value == "-b") {
+            if (std::next(it) == args.end()) {
+                helpAndDie(EXIT_FAILURE);
+            }
 
-                it++;
+            settings.binaryPath = validatePathOrDie(*std::next(it));
+            it++;
+        } else if (value == "--plugin" || value == "-p") {
+            if (std::next(it) == args.end()) {
+                helpAndDie(EXIT_FAILURE);
+            }
 
-                continue;
-            } else if (value == "--plugin" || value == "-p") {
-                if (std::next(it) == args.end()) {
-                    help();
+            settings.pluginPath = validatePathOrDie(*std::next(it));
+            it++;
+        } else if (value == "--help" || value == "-h") {
+            helpAndDie(EXIT_SUCCESS);
+        } else {
+            std::println(stderr, "[ ERROR ] Unknown option '{}' !", *it);
+            helpAndDie(EXIT_SUCCESS);
+        }
+    }
 
-                    return 1;
-                }
+    // Default options
+    if (settings.configPath.empty())
+        settings.configPath = validatePathOrDie(cwd / "test.lua");
+    if (settings.binaryPath.empty())
+        settings.binaryPath = validatePathOrDie(cwd / "../build/Hyprland");
+    if (settings.pluginPath.empty())
+        settings.pluginPath = cwd;
 
-                pluginPath = *std::next(it);
+    return settings;
+}
 
-                try {
-                    pluginPath = std::filesystem::canonical(pluginPath);
+static bool preTestCleanup() {
+    bool failed = false;
 
-                    if (!std::filesystem::is_regular_file(pluginPath)) {
-                        throw std::exception();
-                    }
-                } catch (...) {
-                    std::println(stderr, "[ ERROR ] plugin '{}' doesn't exist!", pluginPath);
-                    help();
+    if (!Tests::killAllWindows()) {
+        NLog::log("{}Internal failure: failed to kill all windows", Colors::RED);
+        failed = true;
+    }
+    if (!Tests::killAllLayers()) {
+        NLog::log("{}Internal failure: failed to kill all layers", Colors::RED);
+        failed = true;
+    }
+    if (getFromSocket("/reload") != "ok") {
+        NLog::log("{}Internal failure: failed to reload", Colors::RED);
+        failed = true;
+    }
+    if (!getFromSocket("/activeworkspace").contains("workspace ID 1 (1)")) {
+        if (getFromSocket("/dispatch hl.dsp.focus({ workspace = '1' })") != "ok") {
+            NLog::log("{}Internal failure: failed to switch to workspace 1", Colors::RED);
+            failed = true;
+        }
+    }
+    if (getFromSocket("/dispatch hl.dsp.cursor.move({ x = 960, y = 540 })") != "ok") {
+        NLog::log("{}Internal failure: failed to reset cursor position", Colors::RED);
+        failed = true;
+    }
 
-                    return 1;
-                }
+    return !failed;
+}
 
-                it++;
+static void runTests(std::map<const char*, CTestCase&>& testCases) {
+    for (auto& [name, tc] : testCases) {
+        // Clean up before every test
+        NLog::log("{}Cleaning up", Colors::YELLOW);
+        (void)preTestCleanup();
+
+        NLog::log("{}Running test {}", Colors::BLUE, name);
+        tc.test();
+        if (tc.failed)
+            NLog::log("{}Test failed: {}", Colors::RED, name);
+        else
+            NLog::log("{}Test passed: {}", Colors::GREEN, name);
+    }
+}
 
-                continue;
-            } else if (value == "--help" || value == "-h") {
-                help();
+static long long countFailed(const std::map<const char*, CTestCase&>& testCases) {
+    long long ans = 0;
+    for (const auto& [_, tc] : testCases) {
+        if (tc.failed)
+            ans++;
+    }
+    return ans;
+}
 
-                return 0;
-            } else {
-                std::println(stderr, "[ ERROR ] Unknown option '{}' !", *it);
-                help();
+int main(int argc, char** argv, char** envp) {
 
-                return 1;
-            }
-        }
-    }
+    std::span<const char*> args{const_cast<const char**>(argv + 1), sc<std::size_t>(argc - 1)};
+    const SSettings        settings = parseSettings(args);
 
     NLog::log("{}launching hl", Colors::YELLOW);
-    if (!launchHyprland(configPath, binaryPath)) {
+    if (!launchHyprland(settings.configPath, settings.binaryPath)) {
         NLog::log("{}well it failed", Colors::RED);
         return 1;
     }
@@ -220,7 +227,7 @@ int main(int argc, char** argv, char** envp) {
     getFromSocket("/output create headless");
 
     NLog::log("{}trying to load plugin", Colors::YELLOW);
-    if (const auto R = getFromSocket(std::format("/plugin load {}", pluginPath)); R != "ok") {
+    if (const auto R = getFromSocket(std::format("/plugin load {}", settings.pluginPath.string())); R != "ok") {
         NLog::log("{}Failed to load the test plugin: {}", Colors::RED, R);
         getFromSocket("/dispatch hl.dsp.exit()");
         return 1;
@@ -228,34 +235,50 @@ int main(int argc, char** argv, char** envp) {
 
     NLog::log("{}Loaded plugin", Colors::YELLOW);
 
-    NLog::log("{}Running main tests", Colors::YELLOW);
+    long long failedTests = 0, totalTests = 0;
 
-    for (const auto& fn : testFns) {
-        EXPECT(fn(), true);
-    }
+    NLog::log("{}Running main tests", Colors::YELLOW);
+    runTests(mainTestCases);
+    failedTests += countFailed(mainTestCases);
+    totalTests += mainTestCases.size();
 
     NLog::log("{}Running protocol client tests", Colors::YELLOW);
+    runTests(clientTestCases);
+    failedTests += countFailed(clientTestCases);
+    totalTests += clientTestCases.size();
 
-    for (const auto& fn : clientTestFns) {
-        EXPECT(fn(), true);
-    }
+    // TODO: the two tests below should not be hardcoded, include them somewhere
 
     NLog::log("{}running plugin test", Colors::YELLOW);
-    EXPECT(testPlugin(), true);
+    if (!testPlugin()) {
+        NLog::log("{}Test failed: plugin test", Colors::RED);
+        failedTests++;
+    } else {
+        NLog::log("{}Test passed: plugin test", Colors::GREEN);
+    }
+    totalTests++;
 
     NLog::log("{}running vkb test from plugin", Colors::YELLOW);
-    EXPECT(testVkb(), true);
+    if (!testVkb()) {
+        NLog::log("{}Test failed: vkb test from plugin", Colors::RED);
+        failedTests++;
+    } else {
+        NLog::log("{}Test passed: vkb test from plugin", Colors::GREEN);
+    }
+    totalTests++;
 
     // kill hyprland
     NLog::log("{}dispatching exit", Colors::YELLOW);
     getFromSocket("/dispatch hl.dsp.exit()");
 
-    NLog::log("\n{}Summary:\n\tPASSED: {}{}{}/{}\n\tFAILED: {}{}{}/{}\n{}", Colors::RESET, Colors::GREEN, TESTS_PASSED, Colors::RESET, TESTS_PASSED + TESTS_FAILED, Colors::RED,
-              TESTS_FAILED, Colors::RESET, TESTS_PASSED + TESTS_FAILED, (TESTS_FAILED > 0 ? std::string{Colors::RED} + "\nSome tests failed.\n" : ""));
+    NLog::log("\nSummary:\n\tPASSED: {}{}{}/{}", Colors::GREEN, totalTests - failedTests, Colors::RESET, totalTests);
+    NLog::log("\tFAILED: {}{}{}/{}", Colors::RED, failedTests, Colors::RESET, totalTests);
+    if (failedTests > 0)
+        NLog::log("{}Some tests failed.", Colors::RED);
 
     kill(hyprlandProc->pid(), SIGKILL);
 
     hyprlandProc.reset();
 
-    return ret || TESTS_FAILED;
+    return failedTests > 0;
 }