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

TST: Attempted test suite fixups #214

Merged
merged 16 commits into from
Oct 19, 2023
Merged

Conversation

tangkong
Copy link
Contributor

@tangkong tangkong commented Oct 2, 2023

Description

Attempting to fix some intermittent test suite failures. The crux of the difficulty here is that the suite consistently passes locally, failing only intermittently on GHA CI

Motivation and Context

  • Uses / vendors Typhos' WeakPartialMethodSlot for applying callbacks within ActionRowWidget

  • In one case opted to try making the slots submitted to BusyCursorThread actual methods, rather than dynamically created functions

  • When pcdsutils is updated, we can remove the vendoring here and rely on pcdsutils' versino

  • There is still a very rare test suite failure, but I cannot reproduce it enough to address it. For the curious, it seems to fail at get_curr_value, the epics-get portion of ActionRowWidget

    • While this is the one place we chose to make the callbacks methods rather than partial slots, I think the issue is more likely due to late timeouts than widgets being garbage collected early

How Has This Been Tested?

GHA, things pass pretty consistently locally

Where Has This Been Documented?

This PR

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

@tangkong
Copy link
Contributor Author

tangkong commented Oct 3, 2023

Since I mistakenly left an assert False statement, gha now sees what I was seeing locally. While all the tests fail, some of them fail + "attempt to access deleted object"

py3.11, pip fails when trying to teardown some PageWidgets, though test suite completes

py3.9, conda fails with a straight up segfault, suite does not complete

@tangkong
Copy link
Contributor Author

tangkong commented Oct 3, 2023

A slightly better result, though failing the first time (py3.10, conda)

Edit: Through 4 retries not once did the entire matrix pass. I declare this "Not good enough"

@tangkong
Copy link
Contributor Author

tangkong commented Oct 4, 2023

Replacing all partial callbacks with WeakPartialMethodSlot cleans up the test suite quite nicely.

image

I may try to add the class to pcdsutils before I merge this, for the sake of not having to return to this

@tangkong
Copy link
Contributor Author

tangkong commented Oct 4, 2023

I'm not sure if I'm supposed to keep hard references to the WeakPartialMethodSlot object. I got a single segfault testing locally, but haven't been able to get one since, so I'm not sure if it's actually a problem. The GHA test suite seems to be passing without issue, so that's not really helpful

@tangkong
Copy link
Contributor Author

tangkong commented Oct 4, 2023

This PR has become somewhat of a sandbox. I've tried storing references to the created WeakPartialMethodSlot instances, which seems to have helped. I've also (maybe naively) added the arguments received from the signal to the _call method, which doesn't seem to break anything.

@klauer
Copy link
Contributor

klauer commented Oct 4, 2023

I'm not sure if I'm supposed to keep hard references to the WeakPartialMethodSlot object

Looks like you figured it out - the answer is yes 👍

@tangkong tangkong changed the title WIP/TST: Attempted test suite fixups TST: Attempted test suite fixups Oct 5, 2023
@tangkong tangkong requested review from ZLLentz and klauer October 5, 2023 21:54
@tangkong tangkong marked this pull request as ready for review October 5, 2023 21:54
@tangkong
Copy link
Contributor Author

This is ready to be reviewed and merged, with pcdsutils getting WeakPartialMethodSlot

ZLLentz
ZLLentz previously approved these changes Oct 19, 2023
Copy link
Member

@ZLLentz ZLLentz left a comment

Choose a reason for hiding this comment

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

I think all these changes are clean and well-motivated
One nitpick about type annotations

Comment on lines 963 to 964
self._sig: EpicsSignalBase = self.data.to_signal()
if self._sig is None:
Copy link
Member

Choose a reason for hiding this comment

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

This bit is confusing from a type hinting perspective and probably gives some of the stricter type checking libraries pause. It's saying "if this thing that is always an EpicsSignalBase is None..."

Maybe this type should be EpicsSignalBase | None and maybe it should be hinted in the object init or in the class definition itself instead of in this method?

Copy link
Contributor

Choose a reason for hiding this comment

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

It might also just be a regular signal, right?

Copy link
Contributor Author

@tangkong tangkong Oct 19, 2023

Choose a reason for hiding this comment

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

Turns out atef.Procedure.Target.to_signal() -> Optional[EpicsSignal] as written currently, so I'll change it to match and move it to the init.

Edit: I suppose that should also just be a base signal as well.

klauer
klauer previously approved these changes Oct 19, 2023
Copy link
Contributor

@klauer klauer left a comment

Choose a reason for hiding this comment

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

Same as @ZLLentz - a bit of class body type hints are probably in order here.

Comment on lines 963 to 964
self._sig: EpicsSignalBase = self.data.to_signal()
if self._sig is None:
Copy link
Contributor

Choose a reason for hiding this comment

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

It might also just be a regular signal, right?

@tangkong tangkong dismissed stale reviews from klauer and ZLLentz via 5d1999b October 19, 2023 20:30
@tangkong tangkong requested review from klauer and ZLLentz October 19, 2023 20:33
@tangkong tangkong merged commit 2c89f45 into pcdshub:master Oct 19, 2023
9 checks passed
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.

3 participants