-
Notifications
You must be signed in to change notification settings - Fork 730
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
Run testsuite directly #6003
Merged
dyfer
merged 16 commits into
supercollider:develop
from
dyfer:topic/sclang-test-runner-clean-pt1
Apr 30, 2023
Merged
Run testsuite directly #6003
dyfer
merged 16 commits into
supercollider:develop
from
dyfer:topic/sclang-test-runner-clean-pt1
Apr 30, 2023
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
QPM is not used to run tests anymore
check for non-dictionary entries allow inlining improve error message don't run inside try {} catch errors in unit test use formatting in terminal limit posting in the setup stage
dyfer
added
comp: testing
UnitTest class, refactors of existing tests, etc.; don't use if just adding tests as part of a PR
comp: CI/CD
continuous integration and deployment (github actions, etc.)
labels
Apr 5, 2023
dyfer
force-pushed
the
topic/sclang-test-runner-clean-pt1
branch
from
April 6, 2023 00:17
85b2387
to
96516cf
Compare
dyfer
force-pushed
the
topic/sclang-test-runner-clean-pt1
branch
from
April 6, 2023 02:15
96516cf
to
4c8271f
Compare
dyfer
force-pushed
the
topic/sclang-test-runner-clean-pt1
branch
from
April 28, 2023 18:44
4c8271f
to
cdc03b4
Compare
to enable serial port tests
Let's disable testing on Windows until issues with the test runner are addressed
this test failed if the host system was under stress
in test_connectionLost
dyfer
force-pushed
the
topic/sclang-test-runner-clean-pt1
branch
from
April 28, 2023 18:55
cdc03b4
to
5a94636
Compare
joshpar
approved these changes
Apr 30, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
comp: CI/CD
continuous integration and deployment (github actions, etc.)
comp: testing
UnitTest class, refactors of existing tests, etc.; don't use if just adding tests as part of a PR
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Purpose and Motivation
This PR changes the procedure of running the test suite by replacing QPM with direct calls to sclang.
The need to set up QPM makes it harder to recreate testsuite environment locally. IMO by running tests directly with sclang it's easier to debug tests as they might behave within the whole testsuite. Additionally QPM uses Python 2 which is deprecated.
Details
test runner
The new test runner is very similar to the test runner used previously in QPM. The changes include removing some
try {}
statements (trying to avoid #232), as well as adjusting data format (see below).QPM vs direct calls to sclang
QPM had the following roles for the purpose of running tests IIUC:
testsuite/classlibrary
to the compile paths1a. adding
API
quark to the compile paths(1), (2), and (3) is now done as 3 separate calls to sclang. (1a) is not needed. (4) is not implemented.
data format for storing test results
Previously json format was used to store test results. This required using the
API
quark when running testsuite. This has been replaced by native SC objects. Additionally, the "proto" file that indicates which tests to run is now not overwritten with the results; instead a separate file is created to store the results.testing procedure
The testing procedure is largely the same as before, and its reliability hasn't improved that much, but I believe this paves the way for further improvements. One outcome of the proposed changes is that the same script used in the CI can be run directly in the IDE with virtually no changes.
reliability
The reliability of the tests themselves is similar to the previous state. However, I believe the test environment is less likely to do "empty passes", like here, which I think was related to running array operations within
try {}
(i.e. the error reported in #232, IIUC). Still, occasional failures do happen: some tests occasionally fail, sclang segfaults, and mysterious crashes occur when trying to run the whole testsuite on Windows.sclang scripts
This PR adopts the main test runner script from qpm and adds two other scripts: one to add
testsuite/classlibrary
to the compiled paths, and one to post the results.The scripts, if run outside of the IDE, uses ANSI color escape codes. This might be problematic if the scripts are run in a terminal that doesn't support these, but I'd argue this is an issue to be addressed if it actually proves problematic to anyone.
Posting results is done using a separate script and is triggered in a separate "step" in GHA. This makes it easier to see the summary of the results. However, in certain cases, if sclang (erroneously) doesn't compile the class library, the posting fails (I've only seen that on Windows). IMO this is something to investigate later.
updated build matrix
As a part of this update, I've moved the Linux tests to run on a build done under Ubuntu 22.04 and GCC 12. NOTE: This also addresses the problem of the
ubuntu-18.04
images being deprecated.EDIT: fixed tests
I initially didn't intend to include any actual fixes to the UnitTests in this PR, but I've found both
TestThreadReschedule
andTestSerialPort
failing/hanging on multiple runs; the included commits updating these two seem to make test suite more reliable.Outstanding issues
test runner script vs
UnitTest.runAll
One could argue that we shouldn't need a separate test runner script and that it should be enough to run
UnitTest.runAll
. The test runner provides the following functionality, not provided byUnitTest.runAll
:We could consider migrating/integrating that functionality into UnitTest.runAll, but I believe that would be something for another PR. In any case, it would be useful to have everything directly in the main repository, as opposed to running with QPM.
documentation
There's only minimal documentation on qpm and running testsuite in the CI (1, 2); no documentation in the helpfiles AFAICT. This PR does not add any documentation... partially because I think this is still a work in progress - we might move the runner script into the UnitTest class, or change it considerably.
If you feel that some documentation should be added to this PR, please let me know. Otherwise I'd just update the two linked wiki entries indicating that we are now running testsuite directly in sclang after this PR is merged.
tests on Windows
Testing should now be possible on all platforms using essentially the same code.
Testing environment on Windows was added and the pipeline seems to work, but it's currently disabled. There seem to be additional issues on Windows that result in sclang's unexpected behavior, which prevents the testuite from ever finishing. I made some progress with that, but I feel it should be saved for another PR.
You can see a small subset of the testsuite running on Windows here.
Since GHA runners offer no audio hardware, the Windows runner installs Jack2 with JackRouter (virtual ASIO soundcard), which is then used by scsynth during tests.
running time of the whole suite
I believe that the running time of the whole suite is similar to previous implementation. However, the overall running time is not being reported next to the number of passed tests, as it has been with qpm implementation. This is something I could add if desired, but IMO it's enough that the time of the testing step is reported in GitHub Actions UI.
Types of changes
DocumentationTo-do list