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

Use CondVar instead of UnitTest.wait in tests #5457

Merged
merged 3 commits into from
May 24, 2021
Merged

Use CondVar instead of UnitTest.wait in tests #5457

merged 3 commits into from
May 24, 2021

Conversation

patrickdupuis
Copy link
Contributor

Purpose and Motivation

Fixes #5366

This PR removes all but one use of UnitTest.wait inside our test suite (the exception being testing that method itself). CondVar is used as a preferred alternative. I've updated the documentation, and added comments and notes pointing users towards CondVar.

I've also re-written the implementation of UnitTest.wait. This method no longer makes use of Condition as this Class is likely going to be deprecated in the future (or at least no longer be used inside the SC class library).

Types of changes

  • Documentation
  • Bug fix
  • Breaking change (maybe?)

To-do list

  • Code is tested
  • All tests are passing
  • Updated documentation
  • This PR is ready for review

@patrickdupuis patrickdupuis added comp: class library SC class library comp: testing UnitTest class, refactors of existing tests, etc.; don't use if just adding tests as part of a PR labels May 24, 2021
@patrickdupuis patrickdupuis added this to the 3.12.0 milestone May 24, 2021
@dyfer
Copy link
Member

dyfer commented May 24, 2021 •
edited
Loading

Thanks @patrickdupuis !

Could you remove the "skip" entries for the fixed tests from the gha_test_run_proto.json?
E.g. remove the following:

supercollider/testsuite/scripts/gha_test_run_proto.json

Lines 19 to 24 in 6c49680

{
"suite":"TestCoreUGens",
"test":"*",
"skip":true,
"skipReason":"UnitTest has bugs (FIXME)"
},

as well as other fixed tests. Once you do, these tests will run in CI in this PR.

@patrickdupuis
Copy link
Contributor Author

@dyfer I've updated the list of skipped tests in the CI. Let's see what happens in GA :)

@dyfer
Copy link
Member

dyfer commented May 24, 2021

@dyfer I've updated the list of skipped tests in the CI. Let's see what happens in GA :)

I don't think we can run TestDocument in the current configuration since we are not running the IDE, so these should stay skipped. The only other test that's failing is TestUnitTest:test_setUpClass_already_happened

@patrickdupuis
Copy link
Contributor Author

I've added those tests back to the skipped list.

@patrickdupuis
Copy link
Contributor Author

Should we re-run the CI a few times just to make sure everything is passing?

@patrickdupuis
Copy link
Contributor Author

I cleaned up the commit history. Let's run the CI one last time before merging.

@patrickdupuis
Copy link
Contributor Author

@dyfer the test that's failing for Linux hasn't happened before. I'd rather just ignore it and merge the PR. What do you think?

@dyfer
Copy link
Member

dyfer commented May 24, 2021 •
edited
Loading

@dyfer the test that's failing for Linux hasn't happened before. I'd rather just ignore it and merge the PR. What do you think?

I'd be fine ignoring it, but I have seen TestSoundFile_Server:test_cue_playNow failing before, albeit rarely. Up to you whether you want to add skipping it, or merging this as is.

@patrickdupuis
Copy link
Contributor Author

That test isn't touched by any changes made in this PR, so I say we ignore it.

dyfer
dyfer approved these changes May 24, 2021
View reviewed changes
Copy link
Member

@dyfer dyfer left a comment

Choose a reason for hiding this comment

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

Thanks!

@patrickdupuis patrickdupuis merged commit a2347eb into supercollider:develop May 24, 2021
@patrickdupuis patrickdupuis deleted the topic/fix-tests branch May 24, 2021 20:35
@dyfer dyfer mentioned this pull request May 24, 2021
Open
20 tasks
@dyfer
Copy link
Member

dyfer commented May 24, 2021

I've updated lists of tests in #5359 and #5360

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@telephon telephon telephon left review comments

@dyfer dyfer dyfer approved these changes

Assignees
No one assigned
Labels
comp: class library SC class library comp: testing UnitTest class, refactors of existing tests, etc.; don't use if just adding tests as part of a PR
Projects
None yet
Milestone
3.12.0
Development

Successfully merging this pull request may close these issues.

Some tests broke due to a change in UnitTest.wait
3 participants
@patrickdupuis @dyfer @telephon