search

sillsdev / chorus

End-user collaboration library via 3-way xml merging and hg dvcs under the hood
6 stars 26 forks source link

Run tests in parallel, one per framework #354

Closed rmunn closed 6 days ago

rmunn commented 3 weeks ago

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 commented 3 weeks ago

Going to cherry-pick the bugfix from https://github.com/sillsdev/chorus/pull/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.

github-actions[bot] commented 3 weeks ago

Test Results

       8 files  +    4     333 suites   - 79   2h 23m 19s :stopwatch: - 22m 31s    988 tests +105     932 :heavy_check_mark: +  72    56 :zzz: +33  0 :x: ±0  3 145 runs   - 895  3 022 :heavy_check_mark:  - 884  123 :zzz:  - 11  0 :x: ±0 

Results for commit 6094025f. ± Comparison against base commit 15f88237.

This pull request removes 1 and adds 106 tests. Note that renamed tests count towards both. ``` SaveAndLoad_10KRecords_CompletesQuickly ``` ``` AddButtonClicked_NewMessageHasContents_NewMessageAppendedToAnnotation After2Syncs_HistoryHas2 After2Syncs_WithFilter_OnlyFilteredItemsShown AsyncLocalCheckIn_GivesGoodResult AsyncLocalCheckIn_NoHgLockWarningWithMultipleWorkers AsyncLocalCheckIn_NoPreviousRepoCreation_Throws BeforeAnySyncing_EmptyHistory CanMakeNotesBarWithOtherFiles CanShowNotesBar CanShowNotesBrowserPage … ```

:recycle: This comment has been updated with latest results.

rmunn commented 3 weeks ago

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 commented 3 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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

Fixed in commit d5cb084.

rmunn commented 2 weeks ago

@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.

rmunn commented 6 days ago

There's a chicken-and-egg issue now, which can only be solved by someone with admin rights on this repo (which I don't have):

image

The build-and-test workflows are still expected to report their status, but this workflow removes them and replaces them with a parallel build matrix of workflows, with different names. Once this PR is merged, we'll be able to change what checks are expected, but GitHub won't allow me to merge this PR until the required status checks have passed, which includes the build-and-test checks that this PR removes.

Someone with admin access will need to check the checkbox to allow merging the PR before all the required checks have completed; I don't have admin access to the Chorus PR so I can't do it myself.

rmunn commented 6 days ago

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.

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.

Everything that uses Chorus now runs on net8.0 or higher.

Co-authored-by: Kevin Hahn kevin_hahn@sil.org