Skip to content

ignore drfmaster-induced test failures #1267

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

Closed
wants to merge 1 commit into from

Conversation

n2ygk
Copy link
Contributor

@n2ygk n2ygk commented Jan 9, 2025

Description of the Change

Errors introduced into unreleased drfmaster are causing test failures. This splits out the drfmaster tests to ignore
errors.

Checklist

  • PR only contains one change (considered splitting up PR)
  • unit-test added
  • documentation updated
  • CHANGELOG.md updated (only for user relevant changes)
  • author name in AUTHORS

@n2ygk n2ygk requested a review from sliverc January 9, 2025 16:14
@n2ygk n2ygk mentioned this pull request Jan 9, 2025
5 tasks
@sliverc
Copy link
Member

sliverc commented Jan 13, 2025

Thanks for the suggestion. We actually removed those ignore outcome deliberately in PR #1256. The reason is that with this approach when our tests fail against drfmaster it is really hard to notice. An error is hidden in a very long log of a job which shows it is successful, so no need to browse through it normally

Previously, our tests were failing against drfmaster, and we did not notice it for many weeks. Thankfully, by chance, I noticed it and was able to fix it in #1251 before DRF got released. So this ignore_outcome is not an option for me as it is almost the same as not testing against drfmaster at all.

We could of course change our configuration the way, that the drfmaster tests will have its own check in the GitHub check list, which we mark as optional. That way we quickly see when drfmaster is failing but it would still be green to merge.

However, this, I feel, is a lot of maintenance work as we have to duplicate configuration from tox into the GitHub action.

Actually this time we got the error quickly fixed upstream and there were no urgent PRs to merge on our end so we were not really blocked. And if we had an urgent PR we could still force merge it.

So I suggest we leave it as is and deal with errors once they occur (which is usually very rarely).

@n2ygk n2ygk closed this Jan 13, 2025
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.

2 participants