Terasology: EventSystemReplayImplTest test failures

[keturn]

The EventSystemReplayImplTest tests are flaky, they've been frequently (but not always!) failing on Jenkins.

Examples: ① ② ③ ④

Not always the same test, but it looks like they're always getting less than they expect.

Those tests all follow this general pattern:

https://github.com/MovingBlocks/Terasology/blob/7bd8d11b9c25fad90c82f4036f100df2e5a550b2/engine-tests/src/test/java/org/terasology/recording/EventSystemReplayImplTest.java#L115-L119

[keturn]

So it's _likely_ that we could change that time window from 10ms to 100ms and the tests would pass much more often. That's easy to do and wouldn't slow the test run down much, so we should probably do that if we can't come up with any better ideas.

It does raise a few questions:

• Why does it take more than 10ms for this event replay system in its little mock environment to process three events?

• Is there a more deterministic way to decide when to end the test? e.g. something along the lines of "run until event queue is empty" or "wait until all these futures have completed." (with some other guard condition on top to make sure that a test doesn't hang the whole test suite by waiting forever)

[llvieira]

Hi, @keturn!

Unfortunately, I can't see the test reports. It is private? it returns default backend - 404.

Time-dependent tests are not deterministic and can be flaky. In 10ms we can get a different number of events depending on the machine we're running those tests. What is the trade-off of just calling eventSystem.process() and removing the time code?

I think @iaronaraujo may have some clue on how to improve it.

[Cervator]

Hi @llvieira ! Great to see you :-)

Yeah those links go to a Jenkins we just retired. You can see the new ones at http://jenkins.terasology.io/teraorg/job/Terasology/job/engine/ - any yellow ball is a build that failed one or more of those tests (more of them on the PR tab)

Here are a couple direct links:

• http://jenkins.terasology.io/teraorg/job/Terasology/job/engine/job/develop/3/testReport/junit/org.terasology.recording/EventSystemReplayImplTest/testReplayStatus__/
• http://jenkins.terasology.io/teraorg/job/Terasology/job/engine/view/change-requests/job/PR-3890/lastCompletedBuild/testReport/

Would eventSystem.process() be comparable to ""run until event queue is empty" or so? That'd probably be a great fit if so.

[llvieira]

Hi @Cervator! I'm very glad to talk to you again. I've been so busy with work, but I hope I can still offer some help to the community.

These links are working, thank you! :smiley:

---

It seems we already have a guard condition to ensure that processing doesn't run forever.
https://github.com/MovingBlocks/Terasology/blob/563c7f0cb93d2b46f1ae2eb73277cd1b69292e93/engine/src/main/java/org/terasology/recording/EventSystemReplayImpl.java#L241-L245
I guess if we just call eventSystem.process(), it may take a little longer to run the tests, but it will be deterministic. I would need to investigate it further to see why we have those time conditions on the tests. Is there someone to help us with this investigation? :octocat:

I'll send a message to @iaronaraujo to see if he can help us answer these questions.

[iaronaraujo]

Hi guys! Since it's been a while I've created these tests, I am not sure this os correcto, but I think I did those tests that way to similares the game behavior, since process is called everytime the game ticks. I think it might be possible to change It to run until the events ends, but this might require some attributes to become public. If that's not desired, changing the time might solve this issue!

[Cervator]

Hey both 👋

If avoiding the time weirdness is a possibility, maybe that's still better than weird somewhat arbitrary pauses?

[Cervator]

FYI I included https://github.com/MovingBlocks/Terasology/commit/2ff56349f18839314f7820e4b8c5df1663c4b841 with a recent PR merge - tested out with repeated builds under the Nanoware fork, no test failures with the longer time window to finish.

We might still want to fix it avoiding pauses and using a deterministic approach, so leaving this issue open for that.