Part of ENG-4436.
We should clean up the uploaded test data if any test fails.
Details
Details
- Launch the blob service with the command: RUST_LOG=blob=trace cargo run -- --sandbox --localstack-url "http://127.0.0.1:4566" --http-port 50053.
- In services/commtest, run yarn run-integration-tests blob. The test should pass.
- Comment out lines 36-44 and 92-99.
- Run the test twice. An error should occur.
- Uncomment lines 92-99 and run the test twice again. This time, the test should pass.
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
Comment Actions
- This can still break - your cleanup code will fail if something has already been deleted, e.g. when assertion in L38-39 fails. Then, the cleanup error will be displayed instead of the actual error. I placed a suggestion on handling this.
But anyway, the desired effect will be achieved - blobs will be deleted so I'm not requesting to rework the whole approach
- Could you replace all occurrences of Err(...)? with return Err(...);?
| services/commtest/tests/blob_integration_test.rs | ||
|---|---|---|
| 90 | Can we do if let Err(test_failure) = test_results { and then print the test_failure message before continuing with cleanup? See my main comment for reasoning | |
| 94–97 | This actually isn't an assertion error but general failure. I'd rather do sth like this if let Err(err) = remove::run(&client, &item).await { eprintln!("failed to clean test data no. {}: {}", i, &err); return Err(err); } Nit: return Err(err) is IMO more readable than doing Err(err)? | |
Comment Actions
I'm planning to refactor this code. I would like to use the Drop trait to handle removing BlobData, but I need to conduct some research to determine if this solution would be thread-safe.
Comment Actions
Unfortunately, I don't have time to fix this diff, so I'll have to abandon it because it requires a refactor.