Write unit test

[huytd]

Hohoho! Welcome, Hacktoberfesters!

I know you will be here so I prepared some easy stuff for you. Enjoy it.

This project is slowly growing big and we're facing the death threat, we have no unit test!

This is a gold mine! You can open as many pull request as you can to cover all of the code we have in unit tests. But remember, each pull request must be done for a whole module (for example, one model per pull request is ok, but if you open 4 pull requests for 4 random function, it will be rejected right away).

Good luck guys!

[Henning-K]

By whole module, do you mean e.g. the whole src/bin/server.rs file but not necessarily the whole content of the src/bin/ folder?

[huytd]

Yeah, it should be file-by-file

[Henning-K]

Is there any reason for the fact that functions in src/bin/server.rs (e.g. register_user) return the diesel query result when all that is checked by the front-end is result != false?

This severely messes with the testability of the functions since some parameters in the return value differ depending on the state of the DB.

There's probably a way around this by invoking diesel database reset at the start (and end?) of each test and running tests with cargo test -- --test-threads=1 which limits the number of threads so that multiple tests don't depend on shared mutable state (aka a recipe for disaster).

[huytd]

bin/server.rs is actually a mess :D since most of the functions is just a route handler, that do a lot of works in it. I think the better solution for this one is to migrate all of the data manipulating away from the routing logic, to make it testable. Then, we should have the integrate testing which will connect to a real database.

[Henning-K]

A user on the #rust-beginners irc recommended a nice approach whereby the functions have the connection set up depending on whether a test is executed or not (through marking with #[cfg(test)] / #[cfg(not(test))]). The test-specific connection is then actually created with begin_test_transaction, meaning the transaction is not committed and the data changes from the multiple concurrent tests don't interfere with each other.

[mdevlamynck]

Why use PgConnetion instead of the trait Connection? You can then use the real PgConnection in production and using a mock type implementing Connection during tests. Plus that would allow to support other databases. That said it may be more boilerplate when writing tests, I'm not yet familiar with this in rust. What's your take on this?

I'll take a shot at testing the models.rs module.

[huytd]

Hey, I didn't know that we have Connection trait, when was it added? It's diesel.rs's built-in?

[qcam]

@mdevlamynck

What would be the pros and cons of using Connection?

[mdevlamynck]

Basically the same as using an interface vs using a concrete type in other languages.

Pros: would allow to:

• write database agnostic applications
• write tests not even touching a real database so you can have:
  • better tests isolation and reproductibily
  • faster tests
  • more control to trigger some database behaviour you want to test how your code responds to

Cons:

• more verbose code fn find<C: Connection>(conn: &C) vs fn find(conn: &PgConnection)
• might be a bit harder to read / debug code if you don't know what concrete type is used behind the trait
• … aaaand it seems it can't actually be done. I get a weird compilation error trying to do that (apparently because the code can't be generic over the Backend trait) and as discused in https://github.com/diesel-rs/diesel/issues/728 it doesn't seem to be a goal of diesel to support generic connnections.

So I guess that's it for this idea.

[Henning-K]

I'm working on the server.rs unit tests and I'm done with all but the feed() function. Since there is another issue open which hints at a rewrite of that function, does it matter if I submit my PR without a test for feed()?