FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

tests: drop dependency on external mock module for newer python by eli-schwartz · Pull Request #574 · html5lib/html5lib-python · GitHub

tests: drop dependency on external mock module for newer python - #574

Merged
ambv merged 3 commits into
html5lib:masterfrom
eli-schwartz:tests-mock-dep
Jan 10, 2024
Merged

tests: drop dependency on external mock module for newer python#574
ambv merged 3 commits into
html5lib:masterfrom
eli-schwartz:tests-mock-dep

Conversation

Copy link
Copy Markdown
Contributor

Starting python 3.3, this is in the stdlib.

Fixes #541

cclauss commented Jan 10, 2024

Copy link
Copy Markdown
Contributor

Please rebase so the tests pass.

Copy link
Copy Markdown
Contributor Author

All things considered I'd rather wait until I have core maintainer review and then rebase once instead of potentially many times.

cclauss commented Jan 10, 2024

Copy link
Copy Markdown
Contributor

If you were a core maintainer, would you feel more comfortable about reviewing a PR with failing tests (current state) or passing tests? https://ci.appveyor.com/project/gsnedders/html5lib-python/history

Comment thread html5lib/tests/test_meta.py Outdated
from mock import Mock
try:
from unittest.mock import Mock
except:

cclauss Jan 10, 2024
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Copy link
Copy Markdown
Contributor Author

If I were a maintainer, and:

  • my tests don't pass because the test runner claims there are fatal errors in the Windows registry
  • the issue has been ongoing for a very long time

I would not appreciate external drive-by contributors pressuring other external contributors to feel guilty about my project which doesn't have passing tests in the first place.

But also, as it happens, yes, I absolutely would review PRs with failing tests! Even if the failing test is a flaw in the contribution itself.

I do this all the time. Then I offer suggestions to the contributors, who may not be fully aware of deeply hidden quirks of the codebase that require specialized knowledge to handle correctly, knowledge which I can provide in order to aid those contributors in getting their PR into a mergeable state.

Sometimes I, gasp, merge PRs with failing CI because the PR fixes one CI error but not all the errors, and CI will never be fixed if you're not willing to fix issues one by one and instead expect it all to be fixed in one fell swoop and the task becomes so intimidating that everyone just gives up and it never gets fixed.

Sometimes I review a PR and say "lgtm as soon as CI passes, please rebase", so that contributors can feel confident that they're not being ignored, and that if they rebase the PR now then they won't be asked to rebase it once a month for the next year only to have it never merged.

In short, yes, I review PRs without regard to whether they have passing CI, because I do not see what passing CI has to do with anything at the review stage.

ambv merged commit 82c2599 into html5lib:master Jan 10, 2024
lixkel pushed a commit to lixkel/html5lib-python that referenced this pull request May 25, 2026
lixkel pushed a commit to lixkel/html5lib-python that referenced this pull request May 25, 2026
lixkel pushed a commit to lixkel/html5lib-python that referenced this pull request May 26, 2026
lixkel pushed a commit to lixkel/html5lib-python that referenced this pull request May 26, 2026
lixkel pushed a commit to lixkel/html5lib-python that referenced this pull request May 26, 2026
lixkel pushed a commit to lixkel/html5lib-python that referenced this pull request May 26, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
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.

use unittest.mock instead of mock

3 participants


Back | FazBrowse Home | New Git URL