build: turn on integration tests in ctest by default

[cosmin]

They can still be skipped by passing -DSKIP_INTEGRATION_TESTS=ON for the build configuration.

[cosmin]

Let's see how long the tests take to run with this on by default.

[cosmin]

Looks like upon test failures the build automatically calls the action-tmate action which then waits for a while for someone to connect via SSH to debug. Unless someone is in the middle of debugging something that seems suboptimal, should we turn this off for now, by deleting the debug environment and only re-adding it as needed?

[cosmin]

Integration tests should pass after #1379

[joeyparrish]

should we turn this off for now, by deleting the debug environment and only re-adding it as needed?

Yes! I should have deleted it long ago. I left it on by mistake.

[joeyparrish]

should we turn this off for now, by deleting the debug environment and only re-adding it as needed?

Yes! I should have deleted it long ago. I left it on by mistake.

Done. I'll rerun the tests here.

[joeyparrish]

I'm fine with this. If you think the actual test run is taking too long in CI, we can defer merging this and consider ways to speed things up.

[joeyparrish]

So here are some factors that I believe we have to sift through in evaluating the timing:

• Startup latency is based on availability of VMs to run our jobs, and is out of our control and not affected by this change. We should look at individual job timing, not overall workflow timing.
• Our cloud-hosted Linux arm64 VMs have some anomalous timing on occasion, even without this PR. A job that runs in 10m might suddenly take 33m on a subsequent run with no changes. This affects the build step, without tests. We should count those as outliers or as cloud infrastructure issues and exclude them.
• While the tests will take longer, we shouldn't look at just the test step timing. If we go from 30s to 2m for testing, I think that's acceptable if we spent 20m building on the same host. I don't think it would be fair to call it a 4x increase in test run timing in CI if the tests always run after a full build.

Happy to debate any of these if you disagree.

[joeyparrish]

Two build workflow runs for comparison:

• This PR, with integration tests on: https://github.com/shaka-project/shaka-packager/actions/runs/8397326254?pr=1381
• Previous PR, without integration tests: https://github.com/shaka-project/shaka-packager/actions/runs/8397311714?pr=1380

[joeyparrish]

Another way to look at this, which may be simpler, is to find the logs of the packager_test_py run, which contain a standalone wall-clock time:

• linux x64 release static: 1/21 Test #1: packager_test_py .................***Failed 5.13 sec
• linux arm64 release static: 1/21 Test #1: packager_test_py .................***Failed 9.99 sec
• win x64 release static: didn't run, see below
• osx x64 release static: 1/21 Test #1: packager_test_py .................***Failed 16.53 sec
• osx arm64 release static: 1/21 Test #1: packager_test_py .................***Failed 5.79 sec

The error from windows logs:

Process not started
 D:/a/shaka-packager/shaka-packager/build/packager/packager_test.py
[unknown error]

I suspect we need to have the test command involve "python3" or something, at least on Windows. We are probably relying on the shebang line for Linux & macOS.

Does packager_test_py quit on the first failure? If so, the timings above are garbage until you merge your fix from #1379.

[cosmin]

I think the problem is that the integration tests rely on reaching into the source tree to get the test media, and make assumptions about the hardcoded path to the source tree.

[joeyparrish]

1: Test command: D:\a\shaka-packager\shaka-packager\build\packager\packager_test.py

I think the test command needs to include "python3" and avoid using the shebang.

[cosmin]

The test is defined using

add_test (NAME packager_test_py
    COMMAND ${PYTHON_EXECUTABLE} packager_test.py
    WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}
  )

so in theory it should be using python3, but it doesn't look like anything sets the Python executable. I think we need to use FindPython and then only add the test if an interpreter is found.

[cosmin]

Down to only one test failing on Windows shared builds, with the app failing to recognized the --strip_parameter_set_nalus flag, although I think this failure exposes a larger problem. Due to default symbol visibility rules on Windows any flags that are declared within the packager library rather than the packager app are not going to be visible to the app, and therefore ignored by Abseil. All of these flags will likely need SHAKA_EXPORT

[cosmin]

Still haven't found a good solution to the abseil flags from the packager shared library not being recognized by the packager app binary. So I went back to fixing what the tests originally tried to do, and skip them if it was built as a shared library. We can circle back to fixing these flags but meanwhile we should enable the integration tests as part of the build.

[joeyparrish]

Could you please also use ${Python3_EXECUTABLE} in packager/tools/CMakeLists.txt and packager/version/CMakeLists.txt in place of python3? I think that might solve #1385