GitHub Actions: add test suite #5332

dyfer · 2021-01-31T04:53:17Z

Purpose and Motivation

This PR implements running our full test suite in GitHub Actions on Linux and macOS. It is another milestone in implementing #5261. Fixes #3132, supersedes #4749.

Result: 3036 TESTS PASSED, as reported by the GH runner.

In the process on working on this, I have discovered numerous issues, some of them are fixed, while others are "patched" in this PR, yet others are still in place and documented below.

qpm

We are using qpm for running the test suite. @patrickdupuis's work on #4749 was a big help to understand how it works.
Qpm uses the following SuperCollider code to actually run the tests. This code reads a list of tests from a JSON file, expands * globs in test names updating the list in the JSON file, and writes the results of the tests to the same file. It needs the API Quark to work.
Qpm has some system-specific code that needed to be changed to work with GHA runners. I'm not sure what is the canonical way of running xvfb, but this should be discussed on a separate issue in the qpm repo. In this PR I am using qpm with fixes I applied to a new branch.
I've encountered mysterious errors (example) as I've started running larger chunks of the test suite. I think there might be some issues in writing large dictionary to the file, and thus made some changes to the SuperCollider code runnings the tests. Particularly the hardcoded wait times are not a proper solution, but allowed to avoid these errors.
I've placed the JSON file with the list of tests in testsuite/scripts. I'd be happy to change that location if desired. Previously the list for Travis was stored in the root directory of the repo, which I wanted to avoid. I've also created a directory testsuite/scripts/run where a copy of the JSON file is made and qpm is operating on that copy.

failing tests

There is a number of failing tests. Some have bugs and simply don't run, some run locally but fail, and some fail only when running as part of the test suite, sometimes when run locally, sometimes only when run in GHA. For some tests I excluded/listed specific methods, for others I excluded all tests for a given class. I also fixed some tests as part of this PR.
Here are the failing tests:

Tests with bugs:

TestCoreUGens
TestDocument
TestEnvGen_server.test_zero_gate_n_off_not_sent
TestFilterUGens
TestFilterUGens
TestMixedBundleTester
TestNode_Server
TestProxyNodeMap
TestServer_boot

Tests failing locally:

TestNodeProxy.test_copy_nodeMapIsCopied
TestNodeProxyBusMapping:test_audiorate_mapping
TestNodeProxyBusMapping:test_audiorate_mapping_elasticity
TestNodeProxy_Server.test_schedAfterFade_notBeforeQuant
TestNodeProxy_Server.test_synthDef_isReleased_afterFree
TestPanAz
TestSCDoc.test_helpSourceDirs_includedExtensions
TestTempoClock.test_nextTimeOnGrid_okAfterMeterChange
TestUnitTest.test_setUpClass_already_happened

Tests failing in qpm, with short descriptions:

TestBus.test_get: UnitTest sometimes fails when run in qpm
TestCondition.*: UnitTest fails when run in qpm
TestDocumentEnvironment.*: UnitTest creates issues when run in qpm
TestDoneActions.test_freeSelfAndResumeNext: UnitTest fails when run in qpm
TestEnvGen_server.test_shapeHold_setEndValue: UnitTest fails when run in qpm on Linux
TestEnvGen_server.test_trig_gate_nonsustain_env: UnitTest fails on supernova in qpm on Linux
TestEnvironmentDocument.*: UnitTest fails when run in qpm
TestLinkClock.*: UnitTest has unknown issues when run in qpm
TestNodeProxyRoles.test_proxyroles_xfade_smoothly: UnitTest fails in qpm
TestOSCFunc.test_argSizeMatching_Succeeds: UnitTest fails when run in qpm on Linux
TestPV_ChainUGen.*: Server might hang after these tests when run in qpm
TestWebView.*: UnitTest sometimes fails when run in qpm on Linux

running tests locally

To run the test suite in qpm locally, one needs to install qpm (note: it requires python 2)
pip install git+https://github.com/supercollider/qpm.git@topic/xvfb-temp-fix
On Linux, add path with scscyth to the PATH, e.g.
export PATH=/path/to/supercollider/build/Install/bin:$PATH
You might need to temporarily move the Extensions and synthdefs folders to avoid running non-built-in tests and introducing issues from 3rd-party server plugins
Run qpm, providing the JSON file, the location of API quark and the test suite itself, for example
cp /path/to/supercollider/testsuite/scripts/gha_test_run_proto.json /path/to/supercollider/testsuite/scripts/run/gha_test_run.json && qpm test.run -l /path/to//supercollider/testsuite/scripts/run/gha_test_run.json --path /path/to/supercollider/build/Install/SuperCollider/SuperCollider.app/Contents/MacOS/sclang --include ~/path/to/API/quark /path/to/supercollider/testsuite/classlibrary

GitHub Actions implementation

I implemented running tests in jobs separate from the building ones. That way it is presented more clearly when the tests pass/fail, as it is listed in the list of jobs. Unfortunately this makes it also a little slower, since the testing stage needs to wait for all build steps to finish.

EDIT: now the qpm output is in color

Types of changes

Bug fix
New feature

To-do list

Code is tested
~~All~~ most tests are passing (see details)
Updated documentation - sort of
This PR is ready for review
Submit an issue listing failing tests (after merging this PR) (done: #Some tests have bugs #5359, Some tests fail when run in CI #5360)

TestAbstractFunction TestOSCBundle TestSanitize TestServer

patrickdupuis · 2021-02-01T20:46:09Z

Glad to see this work moving forward! That mysterious error is one that I stuggled with enourmously. I never could figure out how to replicate it consistently. I hoped supercollider/qpm#10 had fixed the problem.

As for the tests themselves, a while back I had made myself a list of ones that needed to be fixed or didn't make use of the Server correctly. I'll compare what I had found with what you've posted here. Ideally, you would merge this PR with the minimal set of functionning tests. Then, we can start creating PRs which fix problematic tests one at a time.

dyfer · 2021-02-01T23:22:50Z

Glad to see this work moving forward! That mysterious error is one that I stuggled with enourmously. I never could figure out how to replicate it consistently. I hoped supercollider/qpm#10 had fixed the problem.

I have been experiencing this problem still once passed certain number of tests added.
I first removed writing to the JSON file on each iteration, which seemed to help for some tests. After a while and adding even more tests, the problem reappeared. Thinking that this might have to do with some (a)synchronicity in disk access, I added a wait time before subsequent step and before running the tests. That certainly seems like a hack and I don't know what the real reason for the problem is, but the suite does run now.

As for the tests themselves, a while back I had made myself a list of ones that needed to be fixed or didn't make use of the Server correctly. I'll compare what I had found with what you've posted here. Ideally, you would merge this PR with the minimal set of functionning tests. Then, we can start creating PRs which fix problematic tests one at a time.

Mosts tests already pass, as you can see. The tests that reliably fail (or error out) should be easy to fix.
There's about a dozen tests/suites that fail only when run as part of the test suite, either locally, or in CI. These will be the trickiest to fix - some seem to trigger some mysterious errors (maybe there's something funky going on about sclang's memory management in that huge dictionary?).

And yes, for this PR I don't want to focus on the failing tests, but only GHA implementation (including json files location) and qpm changes (though maybe this PR could go as is, and I'd open a separate ticket for qpm? it's all intertwined though).

mossheim · 2021-02-02T22:26:04Z

thanks for this @dyfer and thanks to @patrickdupuis for the work that led up to it (and sorry again that i let your PR stall)! i will review this next after i finish proposing changes for #5295 .

mossheim · 2021-02-10T12:25:44Z

@dyfer just looked this over, thanks for the clear write-up.

preliminary note, mostly for myself: when i run the tests locally on Arch here are some steps that are different from yours:

install qpm with pip2
install API quark with python2 /usr/bin/qpm quark checkout API -l <some path>, default checkout path only makes sense on macOS (~/Library/Application Support)
run with python2 /usr/bin/qpm test.run -l <json> --path <sclang> --include <API> <classlibrary>

mossheim · 2021-02-10T13:00:23Z

regarding the correct way to run a process with xvbf, i found xvfb-run <command>, which seems to handle most of the setup and cleanup itself. i also see it's being used in this github action: https://github.com/GabrielBB/xvfb-action/blob/master/index.js#L23

i think we might consider moving xvfb-related things out of qpm and into CI scripts instead, with a possible note in the documentation of qpm that if you don't want to see GUI windows popping up while the tests run, you can install xvfb-run yourself and run it that way. this is effectively the same as offering a "headless" switch, but requires less explanation, and would also make qpm a little more robust for wayland support. to me this just "feels right" -- GHA starts an xvfb-run process, which provides an environment for qpm, which runs sclang. what do you think?

the failing tests and new workaround code you added to eliminate those mysterious errors are both fine with me, as a temporary solution.

dyfer · 2021-02-11T21:44:33Z

regarding the correct way to run a process with xvbf, i found xvfb-run (...)

i think we might consider moving xvfb-related things out of qpm and into CI scripts instead, (...)

I'm aware of xvfb-run and the dedicated action.
I also think that it would be better to move xvfb-related things out of qpm. To me this configuration seems too system-specific. This seemed to be divergent from qpm's original design though, that's why I wasn't proposing such change. I'll try this and see if it works.

install API quark with python2 /usr/bin/qpm quark checkout API -l , default checkout path only makes sense on macOS (~/Library/Application Support)

Not sure what you meant here. I am using a specific path for the API quark - this could be the copy that's already installed with the user's SC install (i.e. in downloaded-quarks), or cloned manually (which is what I'm doing in GHA). I see that one can use qpm to checkout a quark, but I wasn't using that functionality, just cloned the quark with git clone. Should I be using qpm for checkout?

mossheim · 2021-02-11T21:47:42Z

Not sure what you meant here. I am using a specific path for the API quark - this could be the copy that's already installed with the user's SC install (i.e. in downloaded-quarks), or cloned manually (which is what I'm doing in GHA). I see that one can use qpm to checkout a quark, but I wasn't using that functionality, just cloned the quark with git clone. Should I be using qpm for checkout?

really, these are just notes for myself. :) i just wanted to install the API quark through qpm and remember how to do it in case i tested again later.

dyfer · 2021-02-12T01:47:31Z

I wasn't able to use https://github.com/GabrielBB/xvfb-action
When it is used to run tests, qpm can't find its plugin config - it probably has to do with some environment variables or paths, as the action uses node's exec.exec to run the script... Here's the message I was getting with that action, where qpm failed to load.

So instead I've run xvfb-run manually, though this resulted in separate test steps for Linux and macOS. How does this look?

mossheim

that's fine, i would personally like to avoid using actions for things as simple as this -- each action we rely on is a potential security/maintainability issue.

it all looks good to me, although 10 minutes to run tests feels awfully long. perhaps in addition to fixing broken tests we should also look into reducing the number or scale of long-running tests and test suites.

mossheim · 2021-02-12T13:12:23Z

testsuite/scripts/gha_test_run_proto.json

+      "skipReason":"UnitTest sometimes fails when run in qpm on Linux (FIXME)"
+    }
+  ]
+}


please add a final newline and indent this file at 4 spaces. if you don't mind, i'd prefer seeing "suite" lines before "test" since that feels more intuitive (general to specific)

Done! Re: "test" vs "suite" - I was just following the convention in the previous file, but I like the idea of changing it into "general to specific"

mossheim

thanks! will merge after CI completes

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 Jan 31, 2021

dyfer force-pushed the topic/gha-tests-clean branch 3 times, most recently from ba0797d to 49560c8 Compare February 1, 2021 03:27

testsuite: fix tests

65a493a

TestAbstractFunction TestOSCBundle TestSanitize TestServer

dyfer force-pushed the topic/gha-tests-clean branch from 49560c8 to 561f2da Compare February 1, 2021 04:30

mossheim added this to the 3.12.0 milestone Feb 3, 2021

mossheim self-assigned this Feb 4, 2021

mossheim reviewed Feb 12, 2021

View reviewed changes

dyfer added 3 commits February 12, 2021 09:32

testing: add scripts for qpm

8160df3

gha: syntax cleanup

21c5a9f

gha: add tests

67bf088

dyfer force-pushed the topic/gha-tests-clean branch from 1609d33 to 67bf088 Compare February 12, 2021 17:32

mossheim approved these changes Feb 12, 2021

View reviewed changes

mossheim mentioned this pull request Feb 12, 2021

[CI] use repo test suite on Travis #4749

Closed

mossheim merged commit 7a1aa68 into supercollider:develop Feb 12, 2021

This was referenced Feb 16, 2021

Some tests have bugs #5359

Open

Some tests fail when run in CI #5360

Open

mossheim added comp: testing comp: testing UnitTest class, refactors of existing tests, etc.; don't use if just adding tests as part of a PR and removed comp: testing UnitTest class, refactors of existing tests, etc.; don't use if just adding tests as part of a PR labels Mar 7, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GitHub Actions: add test suite #5332

GitHub Actions: add test suite #5332

dyfer commented Jan 31, 2021 •

edited

Loading

patrickdupuis commented Feb 1, 2021

dyfer commented Feb 1, 2021 •

edited

Loading

mossheim commented Feb 2, 2021

mossheim commented Feb 10, 2021

mossheim commented Feb 10, 2021

dyfer commented Feb 11, 2021 •

edited

Loading

mossheim commented Feb 11, 2021

dyfer commented Feb 12, 2021

mossheim left a comment

mossheim Feb 12, 2021

dyfer Feb 12, 2021

mossheim left a comment

GitHub Actions: add test suite #5332

GitHub Actions: add test suite #5332

Conversation

dyfer commented Jan 31, 2021 • edited Loading

Purpose and Motivation

qpm

failing tests

running tests locally

GitHub Actions implementation

Types of changes

To-do list

patrickdupuis commented Feb 1, 2021

dyfer commented Feb 1, 2021 • edited Loading

mossheim commented Feb 2, 2021

mossheim commented Feb 10, 2021

mossheim commented Feb 10, 2021

dyfer commented Feb 11, 2021 • edited Loading

mossheim commented Feb 11, 2021

dyfer commented Feb 12, 2021

mossheim left a comment

Choose a reason for hiding this comment

mossheim Feb 12, 2021

Choose a reason for hiding this comment

dyfer Feb 12, 2021

Choose a reason for hiding this comment

mossheim left a comment

Choose a reason for hiding this comment

dyfer commented Jan 31, 2021 •

edited

Loading

dyfer commented Feb 1, 2021 •

edited

Loading

dyfer commented Feb 11, 2021 •

edited

Loading