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.

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

test: fix TTML integration test where ordering was changed by #1364 #1379

Merged

Conversation

cosmin
Copy link
Contributor

@cosmin cosmin commented Mar 22, 2024

No description provided.

@cosmin
Copy link
Contributor Author

cosmin commented Mar 22, 2024

We should turn on integration tests in CI now that they are stable

@joeyparrish
Copy link
Member

We should turn on integration tests in CI now that they are stable

It should just be a matter of adding it to CMake, right?

@cosmin
Copy link
Contributor Author

cosmin commented Mar 22, 2024

It's currently skipped in CMake by SKIP_INTEGRATION_TESTS which is ON by default. We could add a separate job just to run the integration tests, either by rebuilding with -DSKIP_INTEGRATION_TESTS=OFF or by directly invoking packager_test.py

@cosmin
Copy link
Contributor Author

cosmin commented Mar 22, 2024

We could also turn them on by default although given the tests take a while to run I think it's convenient to get early feedback from running the unit tests without integration tests.

@joeyparrish
Copy link
Member

I bet changing the default won't seriously increase the job time in CI. I'll do a local test to ballpark it.

@joeyparrish
Copy link
Member

Clean, configure, build, and test w/o integration tests on my workstation at home:
4:49

Clean, configure, build, and test w/ integration tests on my workstation at home:
5:38

An increase of 49s

Clean, configure, build, and test w/o integration tests in fastest CI configuration:
5:47

Clean, configure, build, and test w/o integration tests in slowest CI configuration:
18:07

I would guess that turning it on by default wouldn't delay the last test result by more than 1-3 minutes.

@cosmin
Copy link
Contributor Author

cosmin commented Mar 22, 2024

I can send a pull request to flip this on by default, although the test times on your workstation seem concerning. On my laptop running

ctest --test-dir build -j 8

runs in 18s

@cosmin
Copy link
Contributor Author

cosmin commented Mar 22, 2024

Ah, this includes a full rebuild of everything including all the 3rd party dependencies.

Something still seems odd though, for me the integration tests take about ~5 minutes to run on their own, because the default unit test runner in Python won't parallelize the tests. I guess the difference could come down to single core performance.

@cosmin cosmin requested a review from joeyparrish March 22, 2024 22:56
@cosmin
Copy link
Contributor Author

cosmin commented Mar 22, 2024

sent #1381 to turn on the integration tests and see how it looks in CI

@joeyparrish joeyparrish merged commit b7dd856 into shaka-project:main Mar 25, 2024
36 checks passed
@github-actions github-actions bot added the status: archived Archived and locked; will not be updated label May 24, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 24, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
status: archived Archived and locked; will not be updated
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants