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

Add some 'not tested' marks to avoid CI failure #39664

Merged
merged 1 commit into from
Mar 22, 2025

Conversation

user202729
Copy link
Contributor

@user202729 user202729 commented Mar 10, 2025

As in the title. I don't think there's any advantage in running the test again.

There's only a very small risk of the fixer forget to delete the marker, but it seems like a nonexistent issue (whichever pull request that fix it should also remove the # not tested)

At least for those that doesn't segmentation fault or hang.

Side note: not sure what's a good solution to this. Maybe we can do #39470 instead? (but then it doesn't apply to meson…)

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

⌛ Dependencies

@user202729 user202729 marked this pull request as ready for review March 10, 2025 07:16
@user202729 user202729 added p: CI Fix merged before running CI tests s: needs review labels Mar 10, 2025
Copy link

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

@tobiasdiez
Copy link
Contributor

I'm unsure if "not tested" or "known bug" is more appropriate. The former seems to be used more often when the tests have some side effects (eg a window opens) and thus you normally don't want to run the tests.

Otherwise looks good to me.

@user202729
Copy link
Contributor Author

user202729 commented Mar 11, 2025

My vote would be to mark the failing tests with known bug ...

A test with "known bug" should always fail.

-- #39470 (comment) ?

there's always an option of adding a "known occasional bug" tag or something.

@tobiasdiez
Copy link
Contributor

Suppose you have a isPrime method. You then want to test this method on a random number to cover the huge input space. If you get a wrong result in 1/x cases, this is a bug in my book.

But anyway, let's not get super focused on the terminology. The important thing is that these bugs are documented for users, and this is also achieved by using not tested.

vbraun pushed a commit to vbraun/sage that referenced this pull request Mar 19, 2025
sagemathgh-39664: Add some 'not tested' marks to avoid CI failure
    
As in the title. I don't think there's any advantage in running the test
again.

There's only a very small risk of the fixer forget to delete the marker,
but it seems like a nonexistent issue (whichever pull request that fix
it should also remove the `# not tested`)

At least for those that doesn't segmentation fault or hang. (For those
who do the only solution I can think of is
sagemath#39539 )

Side note: not sure what's a good solution to this. Maybe we can do
sagemath#39470 instead? (but then it
doesn't apply to meson…)

### 📝 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.
- [ ] I have updated the documentation and checked the documentation
preview.

### ⌛ 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#39664
Reported by: user202729
Reviewer(s):
@vbraun vbraun merged commit 588b400 into sagemath:develop Mar 22, 2025
25 of 26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p: CI Fix merged before running CI tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants