Win32 TestHarness #2313
Win32 TestHarness #2313
Conversation
There was a problem hiding this comment.
Good work! I am concerned about what looks like copy/pasting from the existing test harness (this project already suffers from a lot of code duplication) and I'm worried that one of the changes reduced the scope of the testing, but otherwise, this looks to be in good shape! My comments are inline.
It would also be good to hammer this into a working state again before we pull it.
| ./CI/ephemeral-x.sh -w openbox-session ./ci-regression.sh "/tmp/enigma-master" 4 | ||
| if [ "$TRAVIS_OS_NAME" == "linux" ]; then | ||
| export ASAN_OPTIONS=detect_leaks=0; | ||
| ./CI/ephemeral-x.sh -w openbox-session ./ci-regression.sh "/tmp/enigma-master" 4 |
There was a problem hiding this comment.
Another option is to extract /CI/ephemeral-x.sh -w openbox-session to a variable and only set it to anything on Linux.
| @@ -2,7 +2,13 @@ | |||
|
|
|||
| ###### Harness ####### | |||
| if [ "$TEST_HARNESS" == true ]; then | |||
| LINUX_DEPS="$LINUX_DEPS openbox libgtest-dev wmctrl xdotool lcov" | |||
| LINUX_DEPS="$LINUX_DEPS openbox libgtest-dev wmctrl xdotool lcov x11-utils" | |||
| WINDOWS_DEPS="$WINDOWS_DEPS git make rsync mingw-w64-x86_64-toolchain mingw-w64-x86_64-boost mingw-w64-x86_64-pugixml\ | |||
There was a problem hiding this comment.
Wow, that's an impressive deps list. Nice job tracking those down!
| fi | ||
|
|
||
| if [ "$TRAVIS_OS_NAME" == "windows" ]; then | ||
| ./test-runner --gtest_filter=$FILTER_ARG-Regression.image_load_save_test --gtest_output=xml:test-harness-out/TestReport${AGENT_N}.xml |
There was a problem hiding this comment.
Probably better to extract this directory name (test-harness-out) as broadly as possible. Across all scripts and the binary would be nice, but sounds impractical for something so trivial. Just a variable with a comment explaining that this name is used elsewhere would be good.
| @@ -6,6 +6,9 @@ | |||
| #include <gtest/gtest.h> | |||
|
|
|||
| inline void test_common(TestHarness* test_harness, const std::string& name, bool save_image = true) { | |||
| std::string tracemsg = name.substr(name.find_first_of("[")); | |||
| SCOPED_TRACE(tracemsg); | |||
| @@ -131,6 +131,7 @@ OptionsParser::OptionsParser() : _desc("Options") | |||
| ("enigma-root", opt::value<std::string>()->default_value(fs::current_path().string()), "Path to ENIGMA's sources") | |||
| ("codegen-only", opt::bool_switch()->default_value(false), "Only generate code and exit") | |||
| ("run,r", opt::bool_switch()->default_value(false), "Automatically run the game after it is built") | |||
| ("jobs,j", opt::value<int>()->default_value(1), "The number of compile jobs to run simultaneously") | |||
There was a problem hiding this comment.
Thanks for adding this! I think it's up in another PR, actually. Maybe I should merge that before folding this in.
| #include <string_view> | ||
| #include <windows.h> | ||
|
|
||
| std::vector<TestConfig> GetValidConfigs(bool platforms, bool graphics, bool audio, bool collisions, bool widgets, bool network) { |
There was a problem hiding this comment.
Is this copied from the other TestHarness implementation? It would be good to extract the loop shape and either pass in a predicate (e.g. a std::function to accept a config) if we can't reuse the code in its entirety.
| @@ -0,0 +1,6 @@ | |||
| from PIL import ImageGrab | |||
There was a problem hiding this comment.
Is this really the easiest way to screenshot in Windows? Win32 has methods for this. We also have a screen_save function in ENIGMA; this might be a reason to consider relying on it rather than an external routine.
An alternative implementation: In Win32, you create a new DC, select it in a bitmap, then use BitBlt to copy from the main DC (GetDC(Null)) into that. There's a detailed example (with scaling for some reason) here. It does seem to be a lot of code compared to this Python business, but it's probably less error-prone (in terms of likelihood of building on Azure in two weeks).
Still, as utilities go, two lines of python isn't a hefty tax.
| @@ -0,0 +1,10 @@ | |||
| #!/bin/bash | |||
There was a problem hiding this comment.
This is a lot of CI files at the project root... I understand the .yml files need to be there for the CI services to pick up, but can we move the other utilities into a CI folder? (Probably under //CommandLine/ or //CommandLine/testing/)
| @@ -0,0 +1,28 @@ | |||
| @import url('https://fonts.googleapis.com/css2?family=Titillium+Web&display=swap'); | |||
There was a problem hiding this comment.
(This file in particular probably doesn't belong in the root of the repo...)
| @@ -0,0 +1,25 @@ | |||
| %e-yaml | |||
There was a problem hiding this comment.
Custom compiler for the test harness? Neat idea.
Added Win32 TestHarness.
Screenshot uses python Pillow library. Install using system python.