Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FIX: set message crash with long error message #622

Merged
merged 5 commits into from
Oct 11, 2024

Conversation

ZLLentz
Copy link
Member

@ZLLentz ZLLentz commented Oct 10, 2024

Description

  • Fix an issue where the "text is too long" branch of the positioner widget's _set_status_text method would not work if the input was a string. There was an implementation error where we checked the input type but did not fully leverage it.
  • Add a test for regression coverage.

Motivation and Context

  • Tong ran into an issue today where an exception was being raised for what ended up being a limit error, but since the text for the limit error was very long it ended up triggering this bug.

How Has This Been Tested?

Interactively via hotfix + here via unit test

Where Has This Been Documented?

Adding pre-release notes

Pre-merge checklist

  • Code works interactively
  • Code contains descriptive docstrings, including context and API
  • New/changed functions and methods are covered in the test suite where possible
  • Code has been checked for threading issues (no blocking tasks in GUI thread)
  • Test suite passes locally
  • Test suite passes on GitHub Actions
  • Ran docs/pre-release-notes.sh and created a pre-release documentation page
  • Pre-release docs include context, functional descriptions, and contributors as appropriate

@ZLLentz ZLLentz marked this pull request as ready for review October 10, 2024 23:29
@ZLLentz ZLLentz requested a review from tangkong October 10, 2024 23:30
tangkong
tangkong previously approved these changes Oct 11, 2024
Copy link
Contributor

@tangkong tangkong left a comment

Choose a reason for hiding this comment

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

Makes sense to me, just a minor nitpick after I squinted at this classically problematic test suite for a bit



@pytest.mark.no_gc
def test_positioner_widget_long_status_text(motor_widget, qtbot):
Copy link
Contributor

Choose a reason for hiding this comment

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

I had to remind myselfl aoub this no_gc call (from an earlier CI overhaul), but its usage here jives with my recollection of when and where we were applying this

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to add qtbot as a fixture argument to this test? We give qtbot the motor widget in fixture construction, and it's not used to waitUntil or similar in the test body

Copy link
Member Author

Choose a reason for hiding this comment

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

I think you're right, this is probably a copy/paste test case no thinking situation

@tangkong
Copy link
Contributor

conda 3.11 passed on a re-run. I know I've recently advocated to not permitting flaky testes but the thought of doing yet another deep dive on this test suite scares me slightly

@ZLLentz
Copy link
Member Author

ZLLentz commented Oct 11, 2024

We'll definitely need to dive into the flakiness at some point... I might go in eventually and try to make tests simpler whenever possible

Copy link
Contributor

@tangkong tangkong left a comment

Choose a reason for hiding this comment

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

LGTM

@ZLLentz ZLLentz merged commit ae1d1c3 into pcdshub:master Oct 11, 2024
11 checks passed
@ZLLentz ZLLentz deleted the fix_long_pos_msg_crash branch October 11, 2024 19:40
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.

2 participants