Skip to content

Conversation

user202729
Copy link
Contributor

@user202729 user202729 commented Feb 13, 2025

As in the title.

The feature was added in #36938 for the purpose of showing GitHub annotations for test failures, but there wasn't a doctest for it. This adds the test.

📝 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.
  • I have updated the documentation and checked the documentation preview. (no change to documentation preview because private method.)

⌛ Dependencies

Copy link

github-actions bot commented Feb 13, 2025

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

@kwankyu
Copy link
Collaborator

kwankyu commented Feb 28, 2025

I am curious why the text "Failed example" shows twice (once in red and once in black), as seen:

Screenshot 2025-02-28 at 1 03 32 PM

? Is this intended?

@user202729
Copy link
Contributor Author

user202729 commented Mar 1, 2025

It has always been like that, but looking at what the program prints out

 ::error title=Failed example:,file=.../sage/doctest/forker.py,line=12::Failed example:
    doctest_var = 42; doctest_var^2

a reasonable guess is the part being put after title= is the red part, and the rest is the code (annotation body).
Double check.

On the other hand in the downloaded log file only

[error]Failed example:
    doctest_var = 42; doctest_var^2

would show up. This actually slightly inconvenience me in that I can't just download the log and parse it for the file and line number.

(side note: There should probably be more tests for the %0A magic and stuff.)

@user202729 user202729 marked this pull request as draft March 1, 2025 07:56
@kwankyu
Copy link
Collaborator

kwankyu commented Mar 1, 2025

It has always been like that, but looking at what the program prints out

 ::error title=Failed example:,file=.../sage/doctest/forker.py,line=12::Failed example:
    doctest_var = 42; doctest_var^2

a reasonable guess is the part being put after title= is the red part, and the rest is the code (annotation body).

Yes, it seems so. Thanks.

On the other hand in the downloaded log file only

[error]Failed example:
    doctest_var = 42; doctest_var^2

would show up. This actually slightly inconvenience me in that I can't just download the log and parse it for the file and line number.

I agree. Maybe something to be improved later, if possible.

@user202729 user202729 marked this pull request as ready for review July 19, 2025 13:49
@user202729
Copy link
Contributor Author

this tests for whole-file timeout and failed example, although using unittest instead of command-line-based test.

the long time warnings are tested in #40413, so this should be good.

Copy link
Member

@dimpase dimpase left a comment

Choose a reason for hiding this comment

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

LGTM

vbraun pushed a commit to vbraun/sage that referenced this pull request Jul 26, 2025
sagemathgh-39512: Add test for github format of doctest report
    
As in the title.

The feature was added in sagemath#36938 for
the purpose of showing GitHub annotations for test failures, but there
wasn't a doctest for it. This adds the test.

### 📝 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.
- [x] I have created tests covering the changes.
- [x] I have updated the documentation and checked the documentation
preview. (no change to documentation preview because private method.)

### ⌛ 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#39512
Reported by: user202729
Reviewer(s): Dima Pasechnik
vbraun pushed a commit to vbraun/sage that referenced this pull request Jul 26, 2025
sagemathgh-39512: Add test for github format of doctest report
    
As in the title.

The feature was added in sagemath#36938 for
the purpose of showing GitHub annotations for test failures, but there
wasn't a doctest for it. This adds the test.

### 📝 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.
- [x] I have created tests covering the changes.
- [x] I have updated the documentation and checked the documentation
preview. (no change to documentation preview because private method.)

### ⌛ 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#39512
Reported by: user202729
Reviewer(s): Dima Pasechnik
vbraun pushed a commit to vbraun/sage that referenced this pull request Jul 27, 2025
sagemathgh-39512: Add test for github format of doctest report
    
As in the title.

The feature was added in sagemath#36938 for
the purpose of showing GitHub annotations for test failures, but there
wasn't a doctest for it. This adds the test.

### 📝 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.
- [x] I have created tests covering the changes.
- [x] I have updated the documentation and checked the documentation
preview. (no change to documentation preview because private method.)

### ⌛ 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#39512
Reported by: user202729
Reviewer(s): Dima Pasechnik
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants