smogon / pokemon-showdown

Pokémon battle simulator.
https://pokemonshowdown.com
MIT License
4.77k stars 2.79k forks source link

Improving tests for sockets, users, and rooms #4159

Open Morfent opened 6 years ago

Morfent commented 6 years ago

tl;dr testing networking in an optimal way would allow testing the parts of Users/Rooms that interact with sockets so real connections, users, and rooms can be used for tests

Slayer95 commented 6 years ago

User#rename since it relies on a round trip from the server to a client to the login server and back in order to work

No! The login server has already participated by the time rename is called. We just validate the signed assertion in the verifier process and send the reply to the user. So networking isn't really involved here.

Users disconnecting voluntarily while in rooms will make the server try to send |deinit| messages to them well after they've disconnected.

This isn't really a problem. In a multiprocess async world, you just ignore the mistimed message and carry on.

Morfent commented 6 years ago

No! The login server has already participated by the time rename is called. We just validate the signed assertion in the verifier process and send the reply to the user. So networking isn't really involved here.

You're right. Networking with the login server would probably be too insecure and lengthy for tests. I was trying to describe a login test from connect to rename, but that's ridiculous. Testing it piece-meal would make more sense and wouldn't take an eternity to finish

This isn't really a problem. In a multiprocess async world, you just ignore the mistimed message and carry on.

...true, but I feel they can be avoided. They're not something I'd consider a high priority to fix though.

Zarel commented 6 years ago

I mean, you can always mock loginserver.js if you really wanted to test login code. Would probably not be a bad thing to test?

Zarel commented 6 years ago

Wait, login code doesn't use loginserver.js

But honestly we need more integration tests. It's weird that we can pass all our current tests without actually being able to start a battle. Sim bots currently literally spin up a PS instance; we should do that too.