Game Runner thread / subprocess improvements #571

keturn · 2020-05-08T20:34:04Z

This is all about what happens when you push that "⏵ Run Game" button.

Things in scope include:

On failure to launch, let the user know as soon as we do. (Instead of freezing for five seconds first.)
Error messages from the failing game task should appear in the Logs tab by default. improve failed launch UX #550

This branch introduces the use of TestFX in order to test code relying on JavaFX classes.

How to test

All the things that involve starting a game and configuring what happens when it starts and exits.

Outstanding before merging

This branch is in a working state at the moment but there are some things to clean up before it'll be out of draft status.

merge dependency branch test-logs test: use spf4j-slf4j-test to unit test logging #562.
make sure github validation can run the unit tests.
case of process-had-no-error-but-was-done-very fast. (I considered forgetting about this requirement, until I realized that it would be bad in conjunction with the exit-after-launch setting.)
move executor-specific things out of ApplicationController.
the case of log-level = None && exit-after-launch = true

maybe nice to have

Comment if these look important:

gradle task to run tests in headless-mode even when run on a fully graphics-capable dev environment (or flag to existing test task)
update acceptance tests.
yet more docstrings. (I think the more outward-facing methods are documented, but there's not 100% coverage of all private methods and fields)
Windows? There are some unit tests marked as Disabled-on-Windows because I don't know if they'll run there. Will they? Do we need alternate test implementations for Windows? Can any of those tests be covered by cross-platform code instead?
TestGameStarter unit tests for heap-min, heap-max, and log-level.

keturn · 2020-05-09T01:13:45Z

Hmm. Github action is failing with:

TestRunGameTask > testGameOutput() FAILED
    java.lang.RuntimeException at WaitForAsyncUtils.java:301
        Caused by: java.lang.UnsupportedOperationException at GtkApplication.java:173

...you want to give me any other details about that, github action? like which invocation of WaitForAsyncUtils that was, or what the heck is on line 173 of GtkApplication.java?

Well. I guess we add "get tests to run under CI" to the list. It's quite possible there's a thing we have to do to tell TestFX it's in headless mode or something.

keturn · 2020-05-09T01:19:36Z

My copy of GtkApplication.java says that UnsupportedOperationException should come with a message:

            if (libraryToLoad == QUERY_NO_DISPLAY) {
                throw new UnsupportedOperationException("Unable to open DISPLAY");

keturn · 2020-05-11T06:30:29Z

For adding an option to force headless mode, gradle can add system properties to the tests: https://docs.gradle.org/current/dsl/org.gradle.api.tasks.testing.Test.html#org.gradle.api.tasks.testing.Test:systemProperties(java.util.Map)

not sure what the most ergonomic way would be to invoke that. I was thinking of adding a --headless option to the existing test task, but I don't know if we can add fields to existing tasks like that. Maybe we end up making a new headlessTest task that extends the base one?

keturn · 2020-05-11T21:50:03Z

Okay. I added a bunch of javadocs.

Let me know when you see other things I've added in here that still could use documenting.

keturn · 2020-05-26T00:16:52Z

Okay, I think I've fixed the regression where this branch wasn't considering the launch a failure if it came back right away. With that, I think this branch is viable.

There's a little more polish and rigor it could have, described by the checklist in the main body of this issue, but I think a lot of those are negotiable depending on how much weight the reviewers give them.

@skaldarnar & @jdrueckert, I'd like for this branch to not get lost in the land of things-that-almost-were. I think it does a significant amount to improve the sort of experience described in #550, and in my humble opinion leaves the code in a better place technically.

Let me know if the test-logs dependency is a blocker and I can pull that out. I never wanted to get in to a fight with slf4j on the classpath, I was just trying to get to a place where I could do some test-driven development for working on GameRunner's quirks. That now accomplished, I care more about having the application code merged than I do for the mess of groovy code that's trying to manage the dependency conflicts.

skaldarnar

Part 1 (up to src/main/java/org/terasology/launcher/game/GameStarter.java)

I still don't fully understand the details of what's going on, but I guess you can live with merging this now and reacting to changes and requirements in the future. If this becomes a blocker in going forward with Java 14+, we'll throw it out/adapt it/replace it 😉

... and conflicts in build.gradle o.O

skaldarnar · 2020-05-28T22:23:08Z

build.gradle

+    testImplementation('org.spf4j:spf4j-slf4j-test:8.8.+') {
+        because "testable logging"
+    }
+    testImplementation("org.slf4j:slf4j-api:1.7.+!!") {


.+!!" the elder speak to me - what do they want to tell me o.O

1.7.+ prefixed version range, any 1.7.x version

!! strict version. that is, if something else pulled in slf4j-api 1.8, do not consider that to satisfy this dependency, because it does not match 1.7.

oh, that page shows there are ways we could write this out in the DSL instead of the symbol-heavy shorthand:

testImplementation("org.slf4j:slf4j-api") { version { strictly("1.7.+") } }

src/main/java/org/terasology/launcher/game/GameService.java

This is a slf4j provider made to write test assertions against. But there can be only one slf4j provider on your classpath at the same time, otherwise who knows which will be used. the remal.component-metadata plugin identifies a lot of these sorts of conflicts so they don't happen silently. but that doesn't have spf4j-slf4j-test in its list of things it knows about, so we also need to specify that. once you've identified conflicts, you have to resolve them, which takes ten lines for some reason, so I put it in a function. debugging wasn't working very well on scripts included with `apply from`, and that's how I ended up writing a gradle plugin.

with slight refactor to avoid mocking static method on Thread.

…ibility to satisfy both app and test code.

The more I look at it the more it circles back to the current design.

like `hasItems` but it checks from one collection you pass to it, not each one of a variable number of arguments.

so other tests can use jupiter Extension

We use TestFX so the JavaFX Task has an Application Thread to run in.

Replace the GameStarter Interface with Callable<Process>.

…cess termination

…if it exits with error

We don't want thread cancellation to terminate the process.

lgtm-com · 2020-09-07T17:37:32Z

This pull request introduces 1 alert when merging 6667350 into 761f149 - view on LGTM.com

new alerts:

1 for Potential input resource leak

keturn · 2020-09-07T17:59:25Z

There might be an intermittent test failure? because the checks shown on this PR pass, but there's a failure reported on https://github.com/keturn/TerasologyLauncher/actions/runs/243384472

skaldarnar · 2020-10-05T18:06:08Z

Since this PR is so huge I wanted to start with compiling the code locally and running the tests. After some trouble with the unicode characters introduced there these are the test results when executing the suite in IntelliJ on Windows:

... not very satisfying 😞 Running gradlew test from in the Windows git bash shows a similar result:

TestRunGameTask > testExeNotFound() FAILED
    java.lang.AssertionError at TestRunGameTask.java:176

TestFileUtils > testCannotCreateDirectory() FAILED
    java.lang.AssertionError at TestFileUtils.java:58

TestFileUtils > testNoPerms() FAILED
    java.lang.UnsupportedOperationException at TestFileUtils.java:83

43 tests completed, 3 failed, 3 skipped

keturn · 2020-10-05T19:22:32Z

Good to know. There's a lot of platform-specific quirks to both filesystems and process execution, and I haven't had a Windows environment to test in.

and, bleh, that's not a very informative test failure message. Looks like I'll have to get that Windows VM up when I have more disk space.

keturn · 2020-10-05T22:22:34Z

wait, I don't see any TestFileUtils changes in this branch. Do those failures happen for you on the main branch?

keturn · 2020-10-05T23:23:07Z

Continuing from discord:

for PR-2 and PR-3 I'm thinking of adding the feature code/refactoring first, without any tests, and adding the tests in PR-3 only. In github, you can set the target of PR-3 to PR-2, and the two diffs will show a bit cleaner. I can review them separately, we merge them together, and then we merge both to master.... or we can merge both to master on their own if you don't mind having a commit in the main branch without the tests 😉

I feel like I'm misunderstanding something significant about what you're looking for out of this.

From just a GitHub UI perspective, if you're reviewing by file, implementation is already separated from tests, and the src/main/* files are sorted before the src/test/* files.

From a reviewer's perspective, I don't know why a reviewer would ask to review the code without its tests. Knowing what is tested and how is a big concern when I do reviews. Ideally, tests also inform the reader about how the code under test is used and why it has some of the wrinkles it does.

(In practice, I often find that when reading a test I have a hard time identifying the important parts of it among all the other stuff that's boilerplate. I'm interested in getting better at that, in both reading and writing tests, so feedback on test legibility is very welcome here.)

all of which to say I don't know how to implement that PR-splitting suggestion in a way that seems like it will be helpful.

keturn · 2020-10-05T23:48:30Z

Some landmarks for navigating this package:

launcher.game.GameService is the entry point, and the part you looked at previously. It's a javafx.concurrent.Service because that seems to be the javafx abstraction that best fits this, as this thing that has its own lifecycle and is not itself in the tree of UI elements. Using the JavaFX-specific class seemed worth doing because JavaFX has rules about which things do and don't happen on the application thread, and using its implementation of these interfaces helps manage that.
launcher.game.GameStarter retains a lot of its responsibilities from before: It's responsible for knowing how to turn our various options in to a Process with all the right arguments. But the code responsible for monitoring the state of that process has been split out to –
launcher.game.RunGameTask, again using the javafx-specific javafx.concurrent.Task to stay on javafx's good side about how to communicate between threads. This is where we put all the gnarly details about how we decide if the game process is alive and healthy. A chief outcome of this PR is to take that logic that was scattered around GameStarter and the Controller and contain it in this nicely Task-shaped interface.

keturn · 2020-10-06T00:16:40Z

(dis)honorable mention:

StringIteratorInputStream (under src/test/):

TerasologyLauncher/src/test/java/org/terasology/launcher/StringIteratorInputStream.java

Lines 28 to 32 in 6667350

    
            * An {@code InputStream} fed by an {@code Iterator<String>}. 
        
            * <p> 
        
            * Testing something that consumes an {@link InputStream} {@linkplain BufferedReader#lines() line-by-line}? Your test 
        
            * fixture has a list of strings to use as inputs; how hard could it be to get an InputStream that will pass them one 
        
            * at a time?

This is a class that I'm inordinately proud I got to work, and I also fear is the worst for maintainability.

I think the way to get it to go away is to factor out this bit of RunGameTask.monitorProcess

TerasologyLauncher/src/main/java/org/terasology/launcher/game/RunGameTask.java

Lines 129 to 131 in 6667350

    
           // log each line of process output 
        
           var gameOutput = new BufferedReader(new InputStreamReader(process.getInputStream())); 
        
           gameOutput.lines().forEachOrdered(this::handleOutputLine);

that is InputStream → Stream<String> lines into something with its own interface that the tests can provide an alternate implementation of, one that doesn't require running test input through BufferedReader.

The fear I have about getting rid of StringIteratorInputStream is that maybe those ugly little details like "calling .available() has an invisible side-effect so calling it twice in a row behaves differently than just calling it once" will end up being somehow important as we try to deal with edge cases with subprocesses on our various supported platforms, and then we'll want this InputStream implementation we can use to simulate those cases.

Is that paranoid?

skaldarnar

Went through all the files once 💪 😩 - looks good so far, some comments and merge conflicts to resolve, but otherwise we're close to getting this merged!

src/main/java/org/terasology/launcher/game/GameService.java

skaldarnar · 2020-10-06T13:49:24Z

src/main/java/org/terasology/launcher/game/RunGameTask.java

+        if ((!valueSet) && START_MATCH.test(line)) {
+            declareSurvival();
+        }
+        logger.info("Game output: {}", line);


Should we log the game's output on INFO level? How complicated is it to still get the launcher logs itself? I image this could be hard to untangle in case somebody really needs to look at the logs... the game does write log files separately anyways, doesn't it?

I think the short-term answer for this is that the Launcher tab where you see log output is implemented as a log destination, so if we don't re-log stuff it's not visible there.

skaldarnar · 2020-10-06T13:53:14Z

src/main/java/org/terasology/launcher/gui/javafx/FXUtils.java

+     *
+     * @see <a href="https://github.com/TomasMikula/ReactFX/blob/v2.0-M5/reactfx/src/main/java/org/reactfx/util/FxTimer.java">ReactFX</a>
+     */
+    public static final class FxTimer {


As discussed in #586 this may just live in a file on it's own.

src/main/java/org/terasology/launcher/gui/javafx/FXUtils.java

skaldarnar · 2020-10-06T13:58:51Z

src/test/java/org/terasology/launcher/Matchers.java

+     * For example:
+     * <p>
+     * {@code
+     * assertThat(Arrays.asList("foo", "bar", "baz"), hasItemsFrom(List.of("baz", "foo")))


I think in the example it would make sense to highlight that this is not about includes but a weaker form. The list on the right might contain elements that are not present on the left side. In other words, the intersection of the lists interpreted as sets is not empty - is my understanding correct?

Suggested change

* assertThat(Arrays.asList("foo", "bar", "baz"), hasItemsFrom(List.of("baz", "foo")))

* assertThat(Arrays.asList("foo", "bar", "baz"), hasItemsFrom(List.of("faz", "foo")))

skaldarnar · 2020-10-06T14:07:20Z

src/test/java/org/terasology/launcher/game/UnixProcesses.java

+        return "nope" + new Random()
+            .ints(16, 0, 255).mapToObj(
+                i -> Integer.toString(i, Character.MAX_RADIX)
+            )
+            .collect(Collectors.joining());


Maybe less custom coding here?

Suggested change

return "nope" + new Random()

.ints(16, 0, 255).mapToObj(

i -> Integer.toString(i, Character.MAX_RADIX)

)

.collect(Collectors.joining());

return "nope-" + UUID.randomUUID().toString();

oh, this was just me making up garbage names for testing files? Yeah, sure, UUID is fine.

You get less entropy-per-character in UUID.toString() because it's formatted as base-16 and I think even type-4 UUIDs have at least a couple bits that aren't actually random, but I can't really defend the claim that those details are important to this use case.

skaldarnar · 2020-10-06T14:09:16Z

src/test/java/org/terasology/launcher/game/UnixProcesses.java

+                    String.format("for i in $( seq %s ) ; do echo $i ; sleep 1 ; done", seconds)
+            );
+            var proc = pb.start();
+            LoggerFactory.getLogger(SlowTicker.class).debug(" ⏲ Ticker PID {}", proc.pid());


Yep, I'm pretty sure my Windows complained about these nice little icons.... ⏲

Is that what it was! Okay. We should add that character encoding setting to build.gradle if that fixed it.

src/test/java/org/terasology/launcher/StringIteratorInputStream.java

skaldarnar · 2020-10-06T14:19:32Z

src/test/java/org/terasology/launcher/game/TestGameStarter.java

+    static final String GAME_DATA_DIR = "game_data";
+    static final JavaHeapSize HEAP_MIN = JavaHeapSize.NOT_USED;
+    static final JavaHeapSize HEAP_MAX = JavaHeapSize.GB_4;
+    static final LogLevel LOG_LEVEL = LogLevel.INFO;


We just removed LogLevel so this has to be adjusted to use slf4j's Level - should be an easy fix.

skaldarnar · 2020-10-06T14:29:18Z

src/test/java/org/terasology/launcher/game/TestRunGameTask.java

+
+@Timeout(5)
+@ExtendWith(ApplicationExtension.class)
+public class TestRunGameTask {


Also not reviewing this in detail. I still think that testing the game log bridging and introducing complex dependency mangling for that is a bit of overkill, but I'm impressed you pulled this off. As talked about earlier, as long as we can keep these tests without too much maintenance effort and investment I'm fine with having them, but I don't want them to become a burden for other projects 🙃

The dependency-configuration for slf4j testing did turn in to a complete clownshow, yes. I was hoping that by this point in the year, as Java 15 came out and whatnot, we'd see slf4j 2.0 stabilize and then we'd have better options, but... turns out that hoping people you don't know will do the work you want doesn't necessarily get results.

Testing log output seemed important at the time because that was really the only way that code communicated its outputs. That could be something to keep in mind for future iterations: "my slf4j logs in this TextArea" is currently the only user interface to some important information. But as Launcher develops a richer user interface to communicate about its status and troubleshoot failures, it'll also be developing APIs for that information that don't require going through the logger, and we can base tests on those instead.

Back to the present, though: I hope someone can confirm that TestRunGameTask is comprehensible, because this is where all the persnickity stuff about process state and inter-thread communication is. If things go wrong, or we discover other edge-cases on other platforms, this is where the work will be.

I can confirm that it looks comprehensible 😉 it's nicely structured, has comments, and so on. I think I should have put more emphasis on "in detail" rather than "not reviewing" somehow 🙃

skaldarnar

Will double-check on Windows, but otherwise looks fine!

lgtm-com · 2020-10-09T19:00:00Z

This pull request introduces 1 alert when merging 6693419 into 24252ec - view on LGTM.com

new alerts:

1 for Potential input resource leak

lgtm-com · 2020-10-09T20:07:37Z

This pull request introduces 1 alert when merging aa70a8b into 8065ef6 - view on LGTM.com

new alerts:

1 for Potential input resource leak

skaldarnar · 2020-10-09T21:12:39Z

gnarf, this introduces a flaky test... may need to revert this (or fix fast to not become a problem/annoying) for other PRs... retriggering seems to help, which is even more confusing 🙈

TestRunGameTask > testSuccessEvent() FAILED
    org.opentest4j.AssertionFailedError at TestRunGameTask.java:238

QuantumRenderer: shutdown
43 tests completed, 1 failed

See https://github.com/MovingBlocks/TerasologyLauncher/pull/592/checks?check_run_id=1235517318 for a CI run with failed test, test report attached as build artifact.

This reverts commit 81b566c.

keturn force-pushed the game-start-snappy-5 branch from 69dbd2b to 66b1ba4 Compare May 9, 2020 00:02

This was referenced May 9, 2020

tech debt: launcher.settings vs launcher.config #573

Closed

test: use spf4j-slf4j-test to unit test logging #562

Merged

keturn force-pushed the game-start-snappy-5 branch from 469122c to 1cdfd7c Compare May 20, 2020 00:23

This was referenced May 21, 2020

Topic/modularize #577

Closed

provide natives directory to java when launching process #579

Open

keturn marked this pull request as ready for review May 25, 2020 23:50

skaldarnar requested changes May 28, 2020

View reviewed changes

skaldarnar added Topic: Documentation Documentation and technical writing. Type: Enhancement New features or noticable improvements. labels May 28, 2020

keturn added 15 commits June 1, 2020 14:37

maintenance: convert TestGameRunner to using slf4j-test

97a9cac

with slight refactor to avoid mocking static method on Thread.

build.gradle: keep slf4j-api as an explicit dependency, but with flex…

17a74e8

…ibility to satisfy both app and test code.

Work in Progress: RunGameTask comments

c6a7d6d

Work in Progress: RunGameTask tests?

bfed6b3

Work in Progress: We're going to have a GameStarter after all.

431bec3

The more I look at it the more it circles back to the current design.

extract an interface so we can provide a test implementation

9e84bcd

Matcher hasItemsFrom

d3bbf91

like `hasItems` but it checks from one collection you pass to it, not each one of a variable number of arguments.

test: TestGameStarterWIP: junit4 → junit5

f16526e

TestRunGameTask: split junit4 vintage tests

fc8c7f5

so other tests can use jupiter Extension

TestRunGameTask has learned a lot of things about tasks and processes!

c5d6ec9

We use TestFX so the JavaFX Task has an Application Thread to run in.

TestRunGameTask streamlining

67a6fb6

Replace the GameStarter Interface with Callable<Process>.

Work in Progress: TestRunGameTask thinking about cancellation and pro…

0a38ce0

…cess termination

RunGameTask will throw an exception if the process fails to start or …

16e2a5a

…if it exits with error

We don't have a well-defined spec for thread cancellation.

e3c03e9

We don't want thread cancellation to terminate the process.

keturn mentioned this pull request Oct 4, 2020

Linux process doesn't terminate on exiting the launcher #584

Closed

This was referenced Oct 5, 2020

chore[build]: add TestFX dependency #587

Merged

chore: organize GuiUtils and FXUtils into Effects and Dialogs #586

Merged

keturn mentioned this pull request Oct 6, 2020

Support POSIX-style command line options #589

Closed

skaldarnar reviewed Oct 6, 2020

View reviewed changes

skaldarnar mentioned this pull request Oct 6, 2020

guard better against launching when a game already running #575

Open

skaldarnar previously approved these changes Oct 9, 2020

View reviewed changes

Merge remote-tracking branch 'origin/master' into game-start-snappy-5

aa70a8b

skaldarnar dismissed their stale review via aa70a8b October 9, 2020 20:01

skaldarnar approved these changes Oct 9, 2020

View reviewed changes

skaldarnar and others added 4 commits October 9, 2020 22:50

Merge remote-tracking branch 'origin/master' into game-start-snappy-5

07cb1d8

build: set encoding to UTF-8

09859e7

chore: use random UUID for "unavailable" command in tests

6693419

test: disable more tests on Windows

55c74d5

skaldarnar merged commit 81b566c into MovingBlocks:master Oct 9, 2020

skaldarnar added a commit that referenced this pull request Oct 10, 2020

Revert "Game Runner thread / subprocess improvements (#571)"

759424e

This reverts commit 81b566c.

skaldarnar mentioned this pull request Oct 10, 2020

Revert "Game Runner thread / subprocess improvements" #593

Merged

This was referenced Oct 12, 2020

Game Runner improvements (take 2) #597

Closed

Game Runner improvements (take 2) #600

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Game Runner thread / subprocess improvements #571

Game Runner thread / subprocess improvements #571

keturn commented May 8, 2020 •

edited

Loading

keturn commented May 9, 2020

keturn commented May 9, 2020

keturn commented May 11, 2020

keturn commented May 11, 2020

keturn commented May 26, 2020

skaldarnar left a comment

skaldarnar May 28, 2020

keturn May 29, 2020

lgtm-com bot commented Sep 7, 2020

keturn commented Sep 7, 2020

skaldarnar commented Oct 5, 2020 •

edited

Loading

keturn commented Oct 5, 2020

keturn commented Oct 5, 2020

keturn commented Oct 5, 2020

keturn commented Oct 5, 2020

keturn commented Oct 6, 2020 •

edited

Loading

skaldarnar left a comment

skaldarnar Oct 6, 2020

keturn Oct 6, 2020

skaldarnar Oct 6, 2020

skaldarnar Oct 6, 2020

skaldarnar Oct 6, 2020

keturn Oct 6, 2020

skaldarnar Oct 6, 2020

keturn Oct 6, 2020

skaldarnar Oct 6, 2020

skaldarnar Oct 6, 2020

keturn Oct 6, 2020 •

edited

Loading

keturn Oct 6, 2020

skaldarnar Oct 7, 2020

skaldarnar left a comment

lgtm-com bot commented Oct 9, 2020

lgtm-com bot commented Oct 9, 2020

skaldarnar commented Oct 9, 2020 •

edited

Loading

	* assertThat(Arrays.asList("foo", "bar", "baz"), hasItemsFrom(List.of("baz", "foo")))
	* assertThat(Arrays.asList("foo", "bar", "baz"), hasItemsFrom(List.of("faz", "foo")))

Game Runner thread / subprocess improvements #571

Game Runner thread / subprocess improvements #571

Conversation

keturn commented May 8, 2020 • edited Loading

How to test

Outstanding before merging

maybe nice to have

keturn commented May 9, 2020

keturn commented May 9, 2020

keturn commented May 11, 2020

keturn commented May 11, 2020

keturn commented May 26, 2020

skaldarnar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgtm-com bot commented Sep 7, 2020

keturn commented Sep 7, 2020

skaldarnar commented Oct 5, 2020 • edited Loading

keturn commented Oct 5, 2020

keturn commented Oct 5, 2020

keturn commented Oct 5, 2020

keturn commented Oct 5, 2020

keturn commented Oct 6, 2020 • edited Loading

skaldarnar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keturn Oct 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skaldarnar left a comment

Choose a reason for hiding this comment

lgtm-com bot commented Oct 9, 2020

lgtm-com bot commented Oct 9, 2020

skaldarnar commented Oct 9, 2020 • edited Loading

keturn commented May 8, 2020 •

edited

Loading

skaldarnar commented Oct 5, 2020 •

edited

Loading

keturn commented Oct 6, 2020 •

edited

Loading

keturn Oct 6, 2020 •

edited

Loading

skaldarnar commented Oct 9, 2020 •

edited

Loading