Add tests for Genesis Executor and Tasks #91
Conversation
|
My prop tests have run into some intermittent failures. Trying to isolate. There's a good chance they are caused by the test framework, not the tested code. |
|
Mainnet tests are all failing with the error |
|
@SebastienGllmt LMK if there is something I can do to help get this over the line, or if you think I should go a different direction with this. I know it's a little beefy, so I can think of ways to make it more bite-sized if that would help. |
| insert_active_models(db_tx, active_models).await?; | ||
| let mut all_models = Vec::new(); | ||
| for tx_id in tx_ids { | ||
| let models = <entity::prelude::Address as EntityTrait>::find() |
There was a problem hiding this comment.
This approach will be significantly slower than what we had before. We purposely batch as many things as possible in a single DB request because otherwise the DB overhead gets to be really big
I'm not sure what benefit you were trying to achieve by putting them in a tight for loop. Was this purely to get around using exec_many_with_returning? Probably not worth slowing down just to avoid using this function
There was a problem hiding this comment.
IIRC, the reason I was trying to avoid exec_many_with_returning was so I could use an in-memory DB, and, with SeaORM, you can only do in-memory with SQLite, which doesn't support returning.
I generally prioritize testibility over performance--I'd rather be confident in my code than have it be fast and wrong. The best solution here would be to decouple Carp from SeaORM entirely, so you can just inject a mock db.
There was a problem hiding this comment.
Are operations on db_tx the same as making a "DB request"? It's the lookups that had to be iterated over here (without returning) but if they are just looking at what's in the local DatabaseTransaction rather doing a "DB request" for each, I think that might be fine performance-wise.
Part of #84
This adds some test coverage for the Genesis Executor and the associated Tasks.
Probably isn't complete coverage, but sets a good precedence and subsequent tests will help give a fuller coverage as well.
I was forced to abandon the
exec_many_with_returningas it's not supported withsqlite::memory:.This PR also includes some refactoring as a result of the above change.