Skip to content

Conversation

@user202729
Copy link
Contributor

Follow-up to #36938 and #37738 , this PR shows the test failures of ci-meson.yml workflow as GitHub annotations, which has the advantage that you don't need to scroll through the whole log to view the failed tests.

(actually I just search for Failed examples, but this is not foolproof)

Previously only build.yml got this feature.

There's a disadvantage, then the file and line numbers are no longer shown in the downloaded log file, instead only [error] is shown (but this is trivially fixable by printing the format in both old and new ways when --format github is passed). I think if everyone views the annotations instead of reading the raw log anyway, it shouldn't matter too much.

📝 Checklist

  • The title is concise and informative.
  • The description explains in detail what this PR is about.
  • I have linked a relevant issue or discussion.
  • I have created tests covering the changes. (no functionality change)
  • I have updated the documentation and checked the documentation preview. (no documentation change)

⌛ Dependencies

@github-actions
Copy link

Documentation preview for this PR (built with commit 763521d; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

@user202729
Copy link
Contributor Author

Looks like it's working. (before the change, the highlighted part would always be test-long or test-new instead.)

image

@saraedum saraedum requested a review from tobiasdiez February 13, 2025 18:04
@saraedum
Copy link
Member

Looks good to me. But maybe @tobiasdiez (or somebody else) could sign off on this one?

Copy link
Contributor

@tobiasdiez tobiasdiez left a comment

Choose a reason for hiding this comment

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

Thanks!

This has the slight disadvantage that now genuine errors are annotated a few times, but for this the github interface just doesn't give a better solution atm.

@tobiasdiez
Copy link
Contributor

(concerning the two failing tests, can they not be fixed with the alarm-test-helper that you have introduced recently @user202729 )

@user202729
Copy link
Contributor Author

user202729 commented Feb 17, 2025

The alarm test thing is just a convenient wrapper over alarm, but does almost exactly the same thing (plus ensuring the time from interruption to exception thrown isn't too long).

I don't know what's the cause of the test failure, but it says "time out" means even after the "user" press ctrl-C, wait for ≈ 10 minutes, the test still doesn't finish. Of course I've no idea why (last time I check signals cannot be ignored), but that would genuinely be a test failure.

I guess we can try to set two alarms somehow (simulate "user press ctrl-C again if it doesn't work the first time"). If tests still fail at least we get a slightly better idea why they do (more likely deadlock, less likely signal ignored/erroneously caught).

p/s: actually I did get the (supposedly) lll (low-level lock?) deadlock once locally on my machine, but never got around to debug it.

vbraun pushed a commit to vbraun/sage that referenced this pull request Feb 18, 2025
sagemathgh-39513: Show test failures of ci-meson as annotations
    
Follow-up to sagemath#36938 and
sagemath#37738 , this PR shows the test
failures of ci-meson.yml workflow as GitHub annotations, which has the
advantage that you don't need to scroll through the whole log to view
the failed tests.

(actually I just search for `Failed examples`, but this is not
foolproof)

Previously only build.yml got this feature.

There's a disadvantage, then the file and line numbers are no longer
shown in the downloaded log file, instead only `[error]` is shown (but
this is trivially fixable by printing the format in both old and new
ways when `--format github` is passed). I think if everyone views the
annotations instead of reading the raw log anyway, it shouldn't matter
too much.


### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes. (no functionality
change)
- [ ] I have updated the documentation and checked the documentation
preview. (no documentation change)

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
    
URL: sagemath#39513
Reported by: user202729
Reviewer(s): Tobias Diez
@vbraun vbraun merged commit 72c6188 into sagemath:develop Feb 21, 2025
21 of 23 checks passed
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.

4 participants