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

Unit tests do not correctly cleanup the mock server #30

Open
brisberg opened this issue Jun 16, 2020 · 2 comments
Open

Unit tests do not correctly cleanup the mock server #30

brisberg opened this issue Jun 16, 2020 · 2 comments

Comments

@brisberg
Copy link
Contributor

I noticed a glaring issue with how this project is deployed / tested that I'd like to discuss.

The mock server processes/instances are not properly cleaned up between unit test runs. This includes the projects own unit tests as well as any tests run by the project consumers.

Try pulling #29 and run the tests. They result in output like so: output

This indicates that after the test is completed, it still takes several seconds for the child processes to shut down (resulting in the interleaved test/shutdown logging output). Additionally, the driver/storage process does not shut down, and continues to retry attempting to reconnect to the now closed Storage process.

This can be seen here. When the socket connect is ended, it will attempt a reconnect 1 second later into infinity. I don't see a simple way to close this loop, without ending the entire process.


This is what causes the mocha runner to continue well after the tests. Requiring the use of the --exit flag, which was really just making this issue by closing the mocha process and force closing all of the common/storage invocations.

Additionally, the hack in utils/stdhooks.js was suppressing these console errors.


I am concerned that the strategy of this project isn't going to be maintainable enough going forward. As basically screepsserver.js is replicating most of the logic of screeps/launcher/lib/start.js (to launch the sub processes except engine/main), and the logic of screeps/engine/lib/main.js (to run the update tick, except without looping ticks forever, allowing us to control tick progression).

This means that there will be constant drift between screeps/launcher, screeps/engine and this module which we will need to migrate over. This already came up in #23 where extra initialization was required since it had been added to engine/main in the mean time.


Two alternatives that I can see being workable:

  • We launch the mocked server once (at the start of testing)
  • reset the server data between tests
  • and tear it down once at the end.

That way each individual test doesn't need to cleanly exit the server. We could even launch it in a separate process or docker container or something for easy cleanup.

However, this will open the possibility of test data leaking between tests.

Since the whole point here is to produce a "private server" that will only tick when we ask it to, it might be possible to implement this as a Screeps Server Mod instead of a jerryrigged version of launcher/engine:main.

Then we could depend on the real launcher/engine dependencies (or screeps/screeps itself). And still get the tick control without duplicating any logic ourselves.


I am going to start experimenting on making a server mod, but I'd like to hear your thoughts on these issues.

@pyrodogg
Copy link
Member

Honestly, I think any projects of this sort are bound to be fragile due to messing with internals that aren't meant to be exposed.

If it's possible to get similar functionality out of a sever mod it sounds like a more sustainable method. So, good luck with your research.

Addressing #1, Yes, it could be a benefit to add functionality to reset data between tests without having to (in)completely tear down the server and restart it. It could open up data leakage but increased flexibility in composing tests is a benefit in addition to reducing the hanging process spam.

I'm also not sure what could be done about trying to exit more cleanly without obviously messing with even more internals.

@Hiryus
Copy link
Collaborator

Hiryus commented Jun 17, 2020

The two hacks (mocha --exit and suppressing console logs) were done because I didn't find any good way to shutdown the server properly. It is just not designed to shut down gracefully all processes (unless something changed since then).

Adding a cleaning function which could be called in teardown() would be a good improvement though.

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

No branches or pull requests

3 participants