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: wait for conditions during troublesome tests #185

Merged
merged 3 commits into from
Jul 10, 2023

Conversation

tangkong
Copy link
Contributor

@tangkong tangkong commented Jul 6, 2023

Description

Try to fix tests in #169. Changes like these could be spread in many places

Basically just assert things with a timeout to give conditions the ability to settle.

Motivation and Context

#169 , and other tests

How Has This Been Tested?

Locally, and by hitting the GHA re-run button a bunch of times, likely

Where Has This Been Documented?

This PR.

@tangkong tangkong marked this pull request as ready for review July 7, 2023 18:50
@tangkong tangkong requested review from klauer and ZLLentz July 7, 2023 18:50
@tangkong
Copy link
Contributor Author

tangkong commented Jul 7, 2023

I can expand this treatment to other tests, but I also haven't managed to see a test suite failure throughout the course of this PR. If anyone has a better stress test methodology I'd love to try it

@tangkong tangkong changed the title WIP/TST: wait for conditions during troublesome tests TST: wait for conditions during troublesome tests Jul 7, 2023
@ZLLentz
Copy link
Member

ZLLentz commented Jul 7, 2023

One way to stress test this is to use pytest-repeat but that might reveal other test suite issues

pytest --count=1000 tests/bad_tests.py

in ipimb_row.state_label.styleSheet())

wait_until(half_removed)
assert half_removed
Copy link
Member

Choose a reason for hiding this comment

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

I think these are all typos, you meant to assert the function result, right?

Suggested change
assert half_removed
assert half_removed()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hah no wonder it never failed 🤦

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interestingly, locally the removed case can seemingly hang forever (10+ seconds, which is effectively forever for a computer brain)

Will investigate further

lightapp.destination_combo.setCurrentIndex(4) # set current to MEC
# Create mock functions
for row in lightapp.rows:
monkeypatch.setattr(row[0], 'setHidden', Mock())
# Initialize properly with nothing hidden
lightapp.filter()
for row in lightapp.rows:
row[0].setHidden.assert_called_with(False)
qtbot.waitUntil(lambda: row[0].setHidden.assert_called_with(False))
Copy link
Member

@ZLLentz ZLLentz Jul 7, 2023

Choose a reason for hiding this comment

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

How do these statements work? The encapsuled assert_called_with is actually an assert statement. Does it have some undocumented return value?
https://docs.python.org/3/library/unittest.mock.html#unittest.mock.Mock.assert_called_with

Copy link
Member

Choose a reason for hiding this comment

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

Nevermind, I didn't know qtbot would wait until the assertion passed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea this is some fanciness I picked up while filling out the atef test suite.

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'll put my approval in because this seems to help

@tangkong tangkong merged commit cb374e4 into pcdshub:master Jul 10, 2023
7 of 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.

2 participants