Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Game Runner improvements (take 2) #597

Closed
wants to merge 69 commits into from

Conversation

keturn
Copy link
Member

@keturn keturn commented Oct 11, 2020

#571 was reverted due to failing tests in CI. Let's fix that here!

(see #571 for overall branch feature description)

Outstanding:

  • fix flaky tests
  • fix "input resource leak" reported by LGTM

Optional:

  • upgrade slf4j-test. (I don't think it's related to the tests that are flaky, but there is a more recent release out.)

keturn added 30 commits June 1, 2020 14:37
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.
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>.
We don't want thread cancellation to terminate the process.
we don't use the annotation parts of spf4j-slf4j-test enough to keep Vintage around
This does is a spec change, but the best way to avoid this "test failure won't show us TRACE output unless we use the @CollectLogs decorator" is to not rely on TRACE output.

Especially since we want game output more visible anyway, for troubleshooting on launch failure.
keturn and others added 24 commits June 1, 2020 14:43
without this, "quit launcher after start" could be frustrating.
otherwise the logger shows `null`
@lgtm-com
Copy link

lgtm-com bot commented Oct 11, 2020

This pull request introduces 1 alert when merging d6108dd into 5a20d1a - view on LGTM.com

new alerts:

  • 1 for Potential input resource leak

@keturn keturn marked this pull request as draft October 11, 2020 23:53
@keturn keturn changed the title revert revert #571 Game Runner improvements (take 2) Oct 12, 2020
@keturn
Copy link
Member Author

keturn commented Oct 12, 2020

There seem to be at least a couple tests with intermittent failures, including

  • TestRunGameTask.testSuccessEvent

org.opentest4j.AssertionFailedError: iterable contents differ at index [2], expected: <TASK_VALUE_SET=true> but was: <PROCESS_OUTPUT_LINE=null>

  • TestRunGameTask.testFastExitDoesNotResultInSuccess
org.opentest4j.AssertionFailedError:
                  Expected	Actual
   PROCESS_OUTPUT_LINE=null   PROCESS_OUTPUT_LINE=null
   PROCESS_OUTPUT_LINE=null   PROCESS_OUTPUT_LINE=null
           TASK_FAILED=null ❌ ░░░░░░░░░░░░░░░░░░░░
 ==> iterable lengths differ, expected: <3> but was: <2>

These are failing in a very similar way, so there's likely a common cause either in the implementation or the way we're doing the tests.

That testFastExitDoesNotResultInSuccess output says that after handling the outputs, it expects to see the Task end with a failure status, and it doesn't. It doesn't see the Task end with the wrong status (i.e. success) either, its event log just ends.

An intermittent failure suggests a race condition. I thought I built those test cases to be free of race conditions, but apparently not!

Also worth noting on the testFastExit failure is that the assertion it's failing on, comparing their event logs, is after it passed the assertion that says the GameExitTooSoon exception was thrown.

testSuccessEvent is different in that there isn't an exception thrown. We should add the renderer that prints the two lists in columns to that one. It says it didn't see TASK_VALUE_SET when it expected it, but I wonder if it's just ended up switching its entries at spots 2 and 3.

Unconfirmed Speculation

I have a hunch: the events we record from property listeners or javafx event handlers are written from a different thread than the lineSent PROCESS_OUTPUT events, and so it's ambiguous what order they come in.

Not sure yet how we would handle that. We want to somehow synchronize things so that the next line isn't recorded until after any potential event handlers responding to the previous line have finished?

The fastExit test isn't exactly the same in that way, but still could be a race between gameTask.get() returning and its event handler completing. In which case we could make sure there's nothing outstanding in its event queue before making the next assertion?

@keturn
Copy link
Member Author

keturn commented Oct 19, 2020

replaced by #600 (moved PR branch to org repo, but a PR can't change its source branch.)

@keturn keturn closed this Oct 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants