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

docs: Simplify undestanding what's going on #750

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

MaxymVlasov
Copy link
Collaborator

Description of your changes

  • Add notes for contributors
  • Describe what different parts of code does, for simplifying future maintenance if it will be needed

@MaxymVlasov MaxymVlasov requested a review from yermulnik as a code owner January 9, 2025 22:52
tox.ini Outdated Show resolved Hide resolved
@MaxymVlasov MaxymVlasov force-pushed the chore/imporve_python_tooling_docs branch from 5a84944 to 71886dc Compare January 9, 2025 23:00
tox -qq
```

If there are any issues, copy-paste and run the `python3 ...` command to visualize the pytest coverage report.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
If there are any issues, copy-paste and run the `python3 ...` command to visualize the pytest coverage report.
If there are any issues, copy-paste and run the failed `python3 ...` command to visualize the pytest coverage report.

Not sure whether "failed" is correct word in this context. Just want to give a reader a hint on what exactly needs to be copied and run. Please correct as applicable.

Copy link
Collaborator Author

@MaxymVlasov MaxymVlasov Jan 9, 2025

Choose a reason for hiding this comment

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

It always showed, no matter of execution result

Here is example:

➜ tox -qq                                         
==================================== test session starts ====================================
platform linux -- Python 3.10.12, pytest-8.3.4, pluggy-1.5.0
cachedir: .tox/py/.pytest_cache
rootdir: /home/vm/code/0-other/open-source/pre-commit-terraform
configfile: pytest.ini
testpaths: tests/pytest/
plugins: cov-6.0.0, xdist-3.6.1, mock-3.14.0
collected 11 items                                                                          

tests/pytest/_cli_test.py ....                                                        [ 36%]
tests/pytest/terraform_docs_replace_test.py .......                                   [100%]

---------- coverage: platform linux, python 3.10.12-final-0 ----------
Coverage HTML written to dir /home/vm/code/0-other/open-source/pre-commit-terraform/.tox/py/tmp/htmlcov/

Required test coverage of 100% reached. Total coverage: 100.00%

=================================== slowest 10 durations ====================================
0.01s setup    tests/pytest/_cli_test.py::test_known_interrupts[app-runtime-exc]

(9 durations < 0.005s hidden.  Use -vv to show these durations.)
==================================== 11 passed in 0.18s =====================================

To open the HTML coverage report, run

        python3 -Im webbrowser file:///home/vm/code/0-other/open-source/pre-commit-terraform/.tox/py/tmp/htmlcov/index.html

To serve the HTML coverage report with a local web server, use

        python3 -Im http.server --directory cov_html_report_dir 0

➜ 

Copy link
Collaborator

@yermulnik yermulnik Jan 9, 2025

Choose a reason for hiding this comment

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

Ah, I see. Then maybe either not "failed" but "questionable" or not "python3 ..." but "python3 -Im ..."? 🤔

Copy link
Collaborator

Choose a reason for hiding this comment

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

Or "copy-paste and run the python3 ... command from tox -qq output" 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

The coverage HTML report is always produced by default locally. Unless one overrides the pytest posargs when calling tox. Regardless of whether it's a failure or a success. It's just a convenient one-liner to open a coverage report view in one's browser as opposed to doing this manually.

Copy link
Contributor

@webknjaz webknjaz Jan 15, 2025

Choose a reason for hiding this comment

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

Suggested change
If there are any issues, copy-paste and run the `python3 ...` command to visualize the pytest coverage report.
The easiest way to find out what parts of the code base are left uncovered, is to copy-paste and run the `python3 ...` command that will open the HTML report, so you can inspect it visually.

.github/CONTRIBUTING.md Outdated Show resolved Hide resolved
.github/CONTRIBUTING.md Outdated Show resolved Hide resolved
@@ -308,6 +310,7 @@ jobs:
&& format('-- {0}', inputs.tox-run-posargs)
|| ''
}}
# Generate nice picture of passed/failed tests in GHA Job Summary
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
# Generate nice picture of passed/failed tests in GHA Job Summary
# Generate visualization of passed/failed tests in GHA Job Summary

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It actually creates picture (looks like it is .svg)

"Visualization" is too wide for it, especially as locally and in codecov we can get much more visualization options of errors than just this:

image

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, didn't know it's an image 👍🏻 (the "image" fits better than "picture" though)

Copy link
Contributor

Choose a reason for hiding this comment

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

It's a widget: https://github.com/test-summary/action?tab=readme-ov-file#test-summary. Being an SVG is more of a low-level implementation detail.

Copy link
Collaborator Author

@MaxymVlasov MaxymVlasov Jan 14, 2025

Choose a reason for hiding this comment

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

How about next:

Suggested change
# Generate nice picture of passed/failed tests in GHA Job Summary
# Generate nice SVG image of passed/failed tests in GHA Job Summary

I think if needed, additional details can be retrieved from L318, and future RTFM about it

uses: test-summary/[email protected]

Copy link
Contributor

Choose a reason for hiding this comment

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

sounds good

pytest.ini Outdated Show resolved Hide resolved
tox.ini Outdated Show resolved Hide resolved
tox.ini Outdated Show resolved Hide resolved
MaxymVlasov and others added 2 commits January 10, 2025 01:48
Co-authored-by: George L. Yermulnik <[email protected]>
Co-authored-by: George L. Yermulnik <[email protected]>
.github/CONTRIBUTING.md Outdated Show resolved Hide resolved
@@ -292,6 +292,8 @@ jobs:
--quiet
--
python -Im pre_commit install-hooks
# Create GHA Job Summary markdown table of the coverage report
# For details: ../../tox.ini '[testenv]' 'commands_post'
Copy link
Contributor

Choose a reason for hiding this comment

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

This workflow is generic. That's only happening when the env with pytest is being invoked, but not the other envs.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Suggested change
# For details: ../../tox.ini '[testenv]' 'commands_post'
# But only for 'pytest' env in 'tox'.
# For details: ../../tox.ini '[testenv]' 'commands_post'

Something like that?

Copy link
Contributor

Choose a reason for hiding this comment

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

Technically, for both [testenv] and [testenv:pytest] which are currently set up equivalently.

tox.ini Outdated Show resolved Hide resolved
@@ -38,6 +45,7 @@ commands_post =
cov = coverage.Coverage(); \
cov.load(); \
cov.report(file=gh_summary_fd, output_format="markdown")'
# Find path of coverage & testrun report for next step
Copy link
Contributor

Choose a reason for hiding this comment

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

The main effect is not the lookup (the implementation detail) but exposing the step outputs with what's found (the interface that GHA interacts with).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Suggested change
# Find path of coverage & testrun report for next step
# Exposing path location of coverage & testrun report for next step

?

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# Find path of coverage & testrun report for next step
# Expose the coverage & test run XML report paths into GHA

tox.ini Outdated Show resolved Hide resolved
Co-authored-by: 🇺🇦 Sviatoslav Sydorenko (Святослав Сидоренко) <[email protected]>
Copy link

codecov bot commented Jan 14, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.38%. Comparing base (2e6b270) to head (4c207c5).

✅ All tests successful. No failed tests found.

Additional details and impacted files
@@             Coverage Diff             @@
##            master     #750      +/-   ##
===========================================
- Coverage   100.00%   96.38%   -3.62%     
===========================================
  Files            9       10       +1     
  Lines          161      249      +88     
  Branches         5        7       +2     
===========================================
+ Hits           161      240      +79     
- Misses           0        9       +9     
Flag Coverage Δ
CI-GHA 96.38% <ø> (-3.62%) ⬇️
MyPy 91.05% <ø> (?)
OS-Linux 96.38% <ø> (?)
OS-Windows 100.00% <ø> (ø)
OS-macOS 100.00% <ø> (ø)
Py-3.10.11 100.00% <ø> (ø)
Py-3.10.16 100.00% <ø> (?)
Py-3.11.11 100.00% <ø> (?)
Py-3.11.9 100.00% <ø> (ø)
Py-3.12.8 100.00% <ø> (ø)
Py-3.13.1 96.38% <ø> (-3.62%) ⬇️
Py-3.9.13 100.00% <ø> (ø)
Py-3.9.21 100.00% <ø> (ø)
VM-macos-13 100.00% <ø> (ø)
VM-macos-14 100.00% <ø> (ø)
VM-ubuntu-24.04 100.00% <ø> (?)
VM-ubuntu-latest 91.05% <ø> (?)
VM-windows-2025 100.00% <ø> (ø)
pytest 100.00% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@yermulnik yermulnik left a comment

Choose a reason for hiding this comment

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

@webknjaz Please shout out before @MaxymVlasov has this merged as I'm approving with the belief that you're keeping an eye on it 🤝

2. Create and populate a new hook section in [Hooks usage notes and examples](../README.md#hooks-usage-notes-and-examples).

### Work with Python hooks
Copy link
Contributor

Choose a reason for hiding this comment

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

Technically, tox wraps running tests with pytest, but it also is a command runner for other processes, like running pre-commit. Stating that it's just Python would be misleading. Besides, running non-Python tests should probably be wrapped with either pytestortox` directly, making it the unified interface for testing it all the same way across local and CI envs.

Moreover, the word “hooks” is just as misleading because (1) it's a concept of the pre-commit framework while there are other things happening, like (2) running the tests and (3) linters.

Ideally, other processes would also migrate into tox subcommands.

That said, this could be temporarily titled as "Contributing to Python code" or something along the lines.

tox r -qq -e pre-commit -- mypy --all-files
```

Then copy-paste and run the `python3 ...` commands to check the strictest MyPy coverage reports.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not an automated check, it's just a convenience command for opening an HTML file in a browser.

Suggested change
Then copy-paste and run the `python3 ...` commands to check the strictest MyPy coverage reports.
Then copy-paste and run the `python3 ...` commands to inspect the strictest MyPy coverage reports visually.

@@ -43,7 +43,7 @@ doctest_optionflags = ALLOW_UNICODE ELLIPSIS
empty_parameter_set_mark = xfail

faulthandler_timeout = 30

# Make all warnings be errors
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# Make all warnings be errors
# Turn all warnings into errors

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.

3 participants