databricks-sdk-py icon indicating copy to clipboard operation
databricks-sdk-py copied to clipboard

Don't throw `TimeoutError` but rather the last error of retries

Open nfx opened this issue 1 year ago • 1 comments
trafficstars

Changes

If we raise TimeoutError(..) from last_err, pytest displays the timeout error in the summary FAILED ...::test_running_real_assessment_job - TimeoutError: Timed out after 0:06:00, which makes troubleshooting harder during the stress time.

This change will put TimeoutError in the cause attribute of re-raised last error. See more at https://docs.python.org/3/library/exceptions.html#exception-context

Tests

  • [x] make test run locally
  • [ ] make fmt applied
  • [ ] relevant integration tests applied

nfx avatar Jan 11 '24 18:01 nfx

This seems like it reverses the conventional order of chained exceptions. Is this change just to accommodate pytest's printing? In general, I would suggest not changing the "correct" user-facing behavior to accommodate testing infrastructure.

It seems that pytest has had a lot of work recently in improving behavior and display of nested exceptions and exception groups. Perhaps the simplest is manually checking the cause, like in this pytest issue, showing how to catch and test these types of exceptions. Would that help? Are you testing with the most recent pytest? I see there were very recent changes from late last year around exception handling.

jasongrout-db avatar Jan 11 '24 20:01 jasongrout-db