Skip to content

fix: improve default error handling #1379 #1409#1439

Open
Alexj9837 wants to merge 8 commits intomainfrom
chore/improve-default-error-handling-1379-1409
Open

fix: improve default error handling #1379 #1409#1439
Alexj9837 wants to merge 8 commits intomainfrom
chore/improve-default-error-handling-1379-1409

Conversation

@Alexj9837
Copy link
Copy Markdown

Fixes #1379
Fixes #1409

Both issues related to error handling in the REST client.

1379 - Catches 'UnauthorisedAccessErrorexplicitly instead of string-matching onBlueskyRemoteControlError`.

1409 - Expands the exception hierarchy so each
HTTP status code raises a distinct typed exception rather than bundling
everything into BlueskyRemoteControlError.

Tests

  • Update tests to match new exception signatures
  • Add missing coverage for 422 non-list detail response
  • Add missing coverage for streaming error in run command

@Alexj9837 Alexj9837 self-assigned this Mar 10, 2026
@Alexj9837 Alexj9837 force-pushed the chore/improve-default-error-handling-1379-1409 branch from accdd8c to c5ab6b3 Compare March 10, 2026 14:20
@codecov
Copy link
Copy Markdown

codecov bot commented Mar 10, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.28%. Comparing base (e7eae6c) to head (04973f0).
⚠️ Report is 16 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1439      +/-   ##
==========================================
+ Coverage   95.19%   95.28%   +0.08%     
==========================================
  Files          43       43              
  Lines        3119     3136      +17     
==========================================
+ Hits         2969     2988      +19     
+ Misses        150      148       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Alexj9837 Alexj9837 force-pushed the chore/improve-default-error-handling-1379-1409 branch from c5ab6b3 to 883b09e Compare March 11, 2026 09:39
@Alexj9837 Alexj9837 removed their assignment Mar 11, 2026
@Alexj9837 Alexj9837 force-pushed the chore/improve-default-error-handling-1379-1409 branch from 883b09e to 05150ae Compare March 11, 2026 11:06
Copy link
Copy Markdown
Contributor

@ZohebShaikh ZohebShaikh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good only few things can be tidied up

@Alexj9837 Alexj9837 force-pushed the chore/improve-default-error-handling-1379-1409 branch 5 times, most recently from 1374dc7 to 84ab80b Compare March 17, 2026 13:29
@Alexj9837 Alexj9837 marked this pull request as ready for review March 17, 2026 13:32
@Alexj9837 Alexj9837 requested a review from a team as a code owner March 17, 2026 13:32
@Alexj9837 Alexj9837 force-pushed the chore/improve-default-error-handling-1379-1409 branch from 84ab80b to f33a736 Compare March 17, 2026 13:33
with pytest.raises(BlueskyRemoteControlError) as exception:
client.resume()
assert "<Response [400]>" in str(exception)
assert exception.value.args[0]
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are these asserts testing for?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was a leftover from when BlueskyRemoteControlError had a fixed response that broke with the new error messsages. It only checks the body is non-empty. I can remove this or I could add a description to the 400 responses in set_state, like "Cannot transition from {current_state} to {new_state}") and check that instead? might be better for debugging

Alexj9837 and others added 3 commits April 10, 2026 16:25
Poor grammar.

Co-authored-by: Peter Holloway <peter.holloway@diamond.ac.uk>
… having an effect when running plans - you'll still get the Unauthorised request message"
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Improve default error handling Remove stacktrace from error message when not logged into CLI

3 participants