search

smogon / pokemon-showdown-loginserver

MIT License
4 stars 26 forks source link

Allow for external review #6

Closed mia-pi-git closed 1 year ago

monsanto commented 1 year ago

Please add a .editorconfig to this project. I think this adheres to PS' conventions? You can copy it from that repo.

As an aside, I don't care much about using tabs--PS is the only project I ever touch that uses them--so if you all don't like them feel free to use spaces. But if you don't care it is fine as-is. Just as long as there is an editorconfig.

monsanto commented 1 year ago

How does one run the tests? I see there is a compose.yml file. I don't know how to use docker compose and it is not documented anywhere that I see.

I am going to guess that this requires running commands external to the test runner and in that case I would recommend using testcontainers which handles it for you. Take a look at https://github.com/smogon/smogon.com/tree/master/identity/test for an example with MySQL.

mia-pi-git commented 1 year ago

Please add a .editorconfig to this project. I think this adheres to PS' conventions? You can copy it from that repo.

This is done.

How does one run the tests? I see there is a compose.yml file. I don't know how to use docker compose and it is not documented anywhere that I see.

I never got Docker to work, so tests just have to be run manually by pointing your config at the test database and running npx mocha. I'll work on setting up better tests re: your above example.

monsanto commented 1 year ago

I never got Docker to work, so tests just have to be run manually by pointing your config at the test database and running npx mocha.

  • It's not a great workflow. Can we get docker working for you? What OS are you running?
  • I don't like the idea of the tests being dependent on your config file. The configuration file being deeply intertwined with the rest of the code is a PS-ism that I'm not fond of. Instead, I would suggest parameterizing your interfaces with the information you need from the configuration file, and have the main part of the app load the config and pass the appropriate values in. This way, you can use different values in testing.
monsanto commented 1 year ago

Similar to what I said about esbuild for PS, I don't like using stuff like transpilers or file watchers. I see no reason this cannot be an ESM project, just set "type": "module" in package.json and don't put commonjs in your tsconfig. This is a small enough project that there shouldn't be any problem with a node --loader=ts-node/esm dev workflow and a .ts -> .js samedir build for prod