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

Separated test runs by framework to avoid file-locking conflicts #1359

Closed
wants to merge 8 commits into from

Conversation

tombogle
Copy link
Contributor

@tombogle tombogle commented Nov 7, 2024

This change is Reviewable

@tombogle tombogle requested a review from josephmyers November 7, 2024 04:36
@tombogle tombogle self-assigned this Nov 7, 2024
Copy link

github-actions bot commented Nov 7, 2024

LibPalaso Tests

    45 files  ±0      45 suites  ±0   13m 44s ⏱️ + 3m 12s
 4 830 tests +2   4 598 ✅ +5  231 💤 ±0  1 ❌  - 3 
14 008 runs  +6  13 380 ✅ +9  627 💤 ±0  1 ❌  - 3 

For more details on these failures, see this check.

Results for commit 053a640. ± Comparison against base commit 46fa5f0.

♻️ This comment has been updated with latest results.

…version. Prevent rebuild for tests to see if that solves file-locking problem.
…st that version. Prevent rebuild for tests to see if that solves file-locking problem."

This reverts commit b610252.
Instead, try killing lingering test processes
@jasonleenaylor
Copy link
Contributor

.github/workflows/build.yml line 61 at r2 (raw file):

        files: |
          ./test-results/netcoreapp2.1/**/*.trx
          ./test-results/net6.0/**/*.trx

I don't think this change is needed, the glob should pick up all 3 folders.

@jasonleenaylor
Copy link
Contributor

.github/workflows/build.yml line 25 at r2 (raw file):

    - name: Build project - Debug
      run: dotnet build -bl:build.dbg.binlog --configuration Debug

Curious why you wanted to have both a debug & release build?

Copy link
Contributor Author

@tombogle tombogle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 1 files reviewed, all discussions resolved (waiting on @hahn-kev, @jasonleenaylor, and @josephmyers)


.github/workflows/build.yml line 25 at r2 (raw file):

Previously, jasonleenaylor (Jason Naylor) wrote…

Curious why you wanted to have both a debug & release build?

I believe that's what it was doing before. It already did the release build upfront. But then each test run would rebuild it (in debug). But it seems this was maybe part of what was causing file locking issues since the test runs were apparently running in parallel. So now it does the debug build once upfront. Since there is the possibility of it successfully building in release but not in debug or vice versa, it seems safest to try both builds to be sure they both succeed. (Pretty sure most of our builds for other software does this as well.) I'm definitely kind of new to GHA and YAML, so if something I've said seems to contradict reality, feel free to challenge me.


.github/workflows/build.yml line 61 at r2 (raw file):

Previously, jasonleenaylor (Jason Naylor) wrote…

I don't think this change is needed, the glob should pick up all 3 folders.

I think you're right. I think the following commit proved to be the magic bullet.

Copy link
Collaborator

@josephmyers josephmyers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe Kevin is going to leave a more complete review, but the most obvious thing here is that two of the frameworks are bogus. The fact that this passes is concerning. Apparently, false positives (silent failures) can happen if the projects are not marked with IsTestProject. I opened a new PR which utilizes the GHA matrix feature to separate things and marks our projects with the flag. (I needed a separate PR to fiddle with GHA silliness)

#1362

We also need a check to verify that our three supported frameworks actually have at least a certain number of tests run. We can do this in another task.

Note that this may be moot since Kevin and Joseph are working on a "better" approach, but this at least fixes my egregious bug and maybe serves as a baseline.
@josephmyers
Copy link
Collaborator

I'm closing this in favor of using the concurrency built into GHA, #1362

@tombogle tombogle deleted the separate-tests-by-framework branch November 14, 2024 18:19
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.

3 participants