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.

Sign up for GitHub

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

Jump to bottom

Add "informational: true" to .codecov.yml #35544

Merged
merged 2 commits into from
Jun 3, 2023
Merged

Add "informational: true" to .codecov.yml #35544

merged 2 commits into from
Jun 3, 2023

Conversation

kwankyu
Copy link
Collaborator

@kwankyu kwankyu commented Apr 21, 2023 •
edited
Loading

📚 Description

Add informational: true to .codecov.yml to avoid "codecov/project" check failure, as discussed in #35522 (comment)

📝 Checklist

  • The title is concise, informative, and self-explanatory.
  • 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 accordingly.

⌛ Dependencies

@kwankyu kwankyu changed the title Turn on informational in codecove.yml Add "informational: true" to codecove.yml Apr 21, 2023
@kwankyu kwankyu changed the title Add "informational: true" to codecove.yml Add "informational: true" to .codecov.yml Apr 21, 2023
@kwankyu kwankyu mentioned this pull request Apr 22, 2023
Open
1 task
@tornaria
Copy link
Contributor

tornaria commented May 6, 2023

Can we get this in the main branch please?

It seems impossible to have a PR pass checks otherwise; does somebody care about that? Luckily #35552 seems it will be added to rc2 (but it was added to develop branch before and now just removed, wtf)

@kwankyu
Copy link
Collaborator Author

kwankyu commented May 9, 2023

Can we get this in the main branch please?

Please review.

mkoeppe
mkoeppe approved these changes May 9, 2023
View reviewed changes
Copy link
Contributor

@mkoeppe mkoeppe left a comment

Choose a reason for hiding this comment

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

LGTM

@tobiasdiez tobiasdiez mentioned this pull request May 9, 2023
Merged
5 tasks
@tobiasdiez
Copy link
Contributor

Is this hotfix still needed or can we wait for #35632?

@kwankyu
Copy link
Collaborator Author

kwankyu commented May 10, 2023

If codecov/project fails when codecov/patch passes, would that be a reason to invalidate a PR? I think not.

If there is still, even with #35632, a possibility that codecov/project can fail when codecov/patch passes, then we need the fix of this PR.

If there is no possibility for that, then we can drop this fix.

As we are not sure about the possibility, we can wait and see if #35632 suffices to remove the possibility. I will put this PR to "needs info" status.

@tobiasdiez
Copy link
Contributor

project may fail if you remove e.g. doctests or make erroneous changes in the doctest framework (or more generally any change that results in less tests to be run). But in general you want to be notified in such situations.

@tornaria
Copy link
Contributor

Is this hotfix still needed or can we wait for #35632?

Yes. If PR are still failing checks. Please merge this quickly and then if #35632 is shown to fix the underlying issue, this can be reverted.

Having checks pass should be the topmost priority, otherwise the whole CI thing is half-useless.

@kwankyu
Copy link
Collaborator Author

kwankyu commented May 10, 2023

Is this hotfix still needed or can we wait for #35632?

Yes. If PR are still failing checks.

#35632 promises that it won't happen, or so I understand. If it fails us, then we merge this ticket.

vbraun pushed a commit that referenced this pull request May 28, 2023
gh-35632: Use fixed random-seed in the "Build & Test" CI
a80034a 
    
<!-- Please provide a concise, informative and self-explanatory title.
-->
<!-- Don't put issue numbers in the title. Put it in the Description
below. -->
<!-- For example, instead of "Fixes #12345", use "Add a new method to
multiply two integers" -->

### 📚 Description

Related to #35522 and #35544. Should at least fix the changed codecov
between runs.

<!-- Describe your changes here in detail. -->
<!-- Why is this change required? What problem does it solve? -->
<!-- If this PR resolves an open issue, please link to it here. For
example "Fixes #12345". -->
<!-- If your change requires a documentation PR, please link it
appropriately. -->

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. It should be `[x]` not `[x
]`. -->

- [x] The title is concise, informative, and self-explanatory.
- [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 accordingly.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on
- #12345: short description why this is a dependency
- #34567: ...
-->

<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
    
URL: #35632
Reported by: Tobias Diez
Reviewer(s): Matthias Köppe
@kwankyu
Copy link
Collaborator Author

kwankyu commented May 30, 2023

Now that #35632 is merged, PR #35694 still failed in codecov/project, which I cannot understand.

If other PRs continue to fail in codecov/project, we should consider merging this PR.

@tobiasdiez
Copy link
Contributor

That particular PR failed since its base is not the most recent commit to the dev branch (but some older commit). See https://app.codecov.io/gh/sagemath/sage/pull/35694

Coverage data is based on head 841c4fe(1 uploads) compared to base 7404764(2 uploads)

@kwankyu
Copy link
Collaborator Author

kwankyu commented May 30, 2023

Yes, I saw that. So codecov compares the coverage change between the head and the base. The head is just one commit away from the base, and that one commit contains trivial typo fixes. Then why codecov/project fails?

@tobiasdiez
Copy link
Contributor

Because the coverage data for the base was still calculated without the fix of #35632.

@kwankyu
Copy link
Collaborator Author

kwankyu commented May 30, 2023

Ah, that explains it. Thanks.

OK. We then wait for everything to be on track.

@kwankyu
Copy link
Collaborator Author

kwankyu commented May 31, 2023

How about #35562? It failed on codecov/project.

@tobiasdiez
Copy link
Contributor

No idea, but there are one or two other PRs that fail with the same indirect coverage changes, compare:
https://app.codecov.io/gh/sagemath/sage/pull/35699/indirect-changes and https://app.codecov.io/gh/sagemath/sage/pull/35562/indirect-changes

So that means that although the random seed is fixed, there are moving parts in the doctests. Not good.

@kwankyu
Copy link
Collaborator Author

kwankyu commented May 31, 2023

So that means that although the random seed is fixed, there are moving parts in the doctests. Not good.

It is not good that the stability of codecov/project was not obtained by the random seed fix. But perhaps we cannot blame that there are moving parts in doctests.

Then it seems that we will have chronic failing codecov/project. We should consider merging this PR.

@github-actions
Copy link

github-actions bot commented Jun 1, 2023

Documentation preview for this PR (built with commit a13ff86) is ready! 🎉

@vbraun vbraun merged commit de29e70 into sagemath:develop Jun 3, 2023
@mkoeppe mkoeppe added this to the sage-10.1 milestone Jun 3, 2023
@kwankyu kwankyu deleted the make-codecov-report-informational branch December 13, 2023 11:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mkoeppe mkoeppe mkoeppe approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
sage-10.1
Development

Successfully merging this pull request may close these issues.
5 participants
@kwankyu @tornaria @tobiasdiez @mkoeppe @vbraun