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.

Sign up for GitHub

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

Jump to bottom

Fix unit-test fixture teardown #247

Merged
merged 1 commit into from
Nov 24, 2023
Merged

Fix unit-test fixture teardown #247

merged 1 commit into from
Nov 24, 2023

Conversation

eskaur
Copy link
Member

@eskaur eskaur commented Nov 23, 2023

We noticed that unit-tests were failing when run with SDK 2.11. It turns out that we were not tearing down objects in the right order, and SDK 2.11 is less tolerance to such things.

This commit does two things to fix the unit-tests:

  • Make sure that all fixtures that yield frames end up releasing those frames on teardown.
  • Fix a test that created a new Application inside its code while also indirectly depending on the "application" fixture.

@nedrebo
Copy link
Contributor

nedrebo commented Nov 23, 2023

We noticed that unit-tests were failing when run with SDK 2.11. It turns out that we were not tearing down objects in the right order, and SDK 2.11 is less tolerance to such things.

Maybe mention this in the SKD changelog @apartridge? Just a reminder about the lifetime requirements for Zivid::Applicaton. Probably some customer code will hit the same and then it is nice if it is already addressed in the Changelog.

@eskaur
Copy link
Member Author

eskaur commented Nov 23, 2023

We noticed that unit-tests were failing when run with SDK 2.11. It turns out that we were not tearing down objects in the right order, and SDK 2.11 is less tolerance to such things.

Maybe mention this in the SKD changelog @apartridge? Just a reminder about the lifetime requirements for Zivid::Applicaton. Probably some customer code will hit the same and then it is nice if it is already addressed in the Changelog.

Honestly the docstring for Application is outrageously short

https://www.zivid.com/hubfs/softwarefiles/releases/2.6.0+fa0036c5-1/doc/cpp/classZivid_1_1Application.html#details

@apartridge
Copy link
Contributor

We want to first understand why there is a regression in 2.11 compared to 2.10. We will investigate this.

@eskaur
Copy link
Member Author

eskaur commented Nov 23, 2023

Actually the docstring in the header is good. But I can't find the same info on the doxygen page.

@eskaur
Fix unit-test fixture teardown
3a2f765 
We noticed that unit-tests were failing when run with SDK 2.11. It turns
out that we were not tearing down objects in the right order, and SDK
2.11 is less tolerance to such things.

This commit does two things to fix the unit-tests:
- Make sure that all fixtures that yield frames end up releasing those
  frames on teardown.
- Fix a test that created a new Application inside its code while also
  indirectly depending on the "application" fixture.
@eskaur eskaur force-pushed the fix-unit-test-fixture-teardown branch from a593fa8 to 3a2f765 Compare November 23, 2023 13:40
@eskaur eskaur merged commit b912515 into master Nov 24, 2023
20 checks passed
@eskaur eskaur deleted the fix-unit-test-fixture-teardown branch November 24, 2023 08:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@apartridge apartridge apartridge approved these changes

@nedrebo nedrebo Awaiting requested review from nedrebo

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@eskaur @nedrebo @apartridge