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

Run tests in parallel, one per framework #354

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

rmunn
Copy link
Contributor

@rmunn rmunn commented Oct 29, 2024

Now that tests run on three frameworks (net461, net6.0, net8.0), we can save a lot of build time by running each framework's tests in parallel.


This change is Reviewable

@rmunn rmunn self-assigned this Oct 29, 2024
@rmunn
Copy link
Contributor Author

rmunn commented Oct 29, 2024

Going to cherry-pick the bugfix from #356 in this PR and see if that makes the failing Linux tests pass. If it does, then we should merge #356 first, then this one after #356 is successfully merged.

Copy link

github-actions bot commented Oct 29, 2024

Test Results

     12 files     541 suites   3h 46m 5s ⏱️
   988 tests    932 ✔️   56 💤 0
5 171 runs  4 981 ✔️ 190 💤 0

Results for commit 1eaa017.

♻️ This comment has been updated with latest results.

@rmunn
Copy link
Contributor Author

rmunn commented Oct 30, 2024

Okay, now the tests are passing but the dotnet test run is still exiting with exit code 1 for some reason?

2024-10-29T11:46:47.1463558Z Passed!  - Failed:     0, Passed:   966, Skipped:    20, Total:   986, Duration: 25 m 49 s - LibChorus.Tests.dll (net6.0)
2024-10-29T11:46:47.2241280Z ##[error]Process completed with exit code 1.

I'll investigate why dotnet test doesn't like what we're doing. But it's probably related to running the tests in parallel, because the same code passes in PR #356.

@rmunn
Copy link
Contributor Author

rmunn commented Oct 30, 2024

Looks like this might be running into dotnet/sdk#29742 where running dotnet test --no-build after dotnet build is failing on ubuntu-latest runners. As a workaround, I'm going to try just running dotnet test without the --no-build parameter; this will build twice, but it should then stop complaining about DLLs being "invalid arguments" (?).

@hahn-kev
Copy link
Contributor

hahn-kev commented Nov 5, 2024

Talked about this with Robin, the issue is that Chorus.Tests only run against net461, so when we say dotnet test -f net8.0 it fails because Chorus.Tests does not support net8.0. To fix this we will split out our dotnet test command to be explicit about which test project we are running, then we can skip Chorus.Tests when we are not running our net461 tests. As for the tests filter which is why we split linux and windows, we can turn that into a github action expression, this will let us avoid having 6 commands, 3 per project and 2 per os.

As for the --no-build issue, it doesn't look like that's effecting us in our other tests, so I assume it's fine here.

@rmunn
Copy link
Contributor Author

rmunn commented Nov 5, 2024

Turns out we don't actually need the test filter to be different between Linux and Windows anymore, since we're skipping running net461 tests on Linux, and all the tests that needed the filter are net461 (and not likely to change since they use Windows Forms). So I chose to keep it simple and not have any filter differences (and just skip the SkipOnBuildServer tests everywhere).

@hahn-kev
Copy link
Contributor

hahn-kev commented Nov 6, 2024

nice work, though it looks like the Build Windows installers job is now failing.

Also it looks like no tests are actually running for net461, so I'm going to look into that

@rmunn
Copy link
Contributor Author

rmunn commented Nov 7, 2024

nice work, though it looks like the Build Windows installers job is now failing.

Fixed in commit d5cb084.

rmunn and others added 14 commits November 7, 2024 15:15
Now that tests run on three frameworks (net461, net6.0, net8.0), we can
save a lot of build time by running each framework's tests in parallel.
If running with `--no-build`, then `dotnet test` complains about DLLs
being invalid arguments, resulting in an exit code of 1 even if all
tests passed. This then makes GHA think the build was a failure. The
discussion in the dotnet SDK issue suggests removing `--no-build` as
one possible workaround for this issue.
Otherwise we get errors about the dotnet restore not being done for the
right framework.
Also, forgot to specify target framework on Windows; no wonder it was
taking so much longer!
Since ChorusMerge runs on net461;net6.0;net8.0, the ChorusMerge tests
should be run on all the same frameworks.
Also only run Chorus.Tests on net461, since it's not defined for net6.0
and net8.0.
Now that the installer job is not running in the same job matrix as the
tests, we need to re-run the dotnet restore and build steps again.
@rmunn rmunn force-pushed the chore/run-tests-in-parallel branch from d5cb084 to 1eaa017 Compare November 7, 2024 08:16
@rmunn
Copy link
Contributor Author

rmunn commented Nov 7, 2024

@hahn-kev - Rebased on top of master after merging the other two PRs, so my bugfixes to Platform.IsMono are now part of this PR's base and it's no longer giving failures on Linux.

@jasonleenaylor - Please let us know if this causes any problems for the Chorus installer build.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tweak GitHub Actions workflow to run tests on multiple TargetFrameworks in parallel
2 participants