Details
cd services yarn run-integration-tests backup
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| services/commtest/tests/backup/pull_backup.rs | ||
|---|---|---|
| 52 ↗ | (On Diff #13884) | we should be able to combine this match statement with the next one and terminate early if the right ID isn't provided for the response_data type |
| 53–57 ↗ | (On Diff #13884) | i prefer we just say BackupID(id) and LogID(id). these are temporary variables so it's ok to use id in both |
| 68 ↗ | (On Diff #13884) | shouldn't the asserts and expects be in the actual test function? |
| 80 ↗ | (On Diff #13884) | shouldn't the asserts and expects be in the actual test function? also, we should use assert_eq here |
| services/commtest/tests/backup/pull_backup.rs | ||
|---|---|---|
| 52 ↗ | (On Diff #13884) | I wanted to do it like this but couldn't hack it. |
| 53–57 ↗ | (On Diff #13884) | right |
| 68 ↗ | (On Diff #13884) | Why? It's much easier to do it this way. Otherwise, we would have to terminate early, set a proper error, and panic in the outer scope. If we panic anyway, then why not do this here? |
| services/commtest/tests/backup/pull_backup.rs | ||
|---|---|---|
| 68 ↗ | (On Diff #13884) | Overall, test assertions should be in test code and not in the utils. This case is a little different because the point of the whole binary is to test the system, so it might be ok. If this code had any other use except being a part of a test, I would insist on avoiding the assertion and returning error instead. Even now, I think it would be better to not panic here and instead return an error, but we can keep the current solution if the change is really that complicated. This function already returns a Result so it shouldn't be that hard.
By panicking here we force a caller to use this particular approach to error. There are some other options, e.g. ignoring error, inspecting it, asserting something about it, forwarding it - by panicking we limit the available possibilities for a caller. Please note that a test could return Result instead of asserting - using this function makes it a lot more restricted. |
It still feels like an anti-pattern to have all these asserts and panics in a subroutine, but as @palys-swm said, it's not a big deal since this whole package is just for testing
| services/commtest/tests/backup/pull_backup.rs | ||
|---|---|---|
| 68 ↗ | (On Diff #13884) | Fair enough, I'm going to change this |