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
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 35 additions & 7 deletions .github/CONTRIBUTING.md
Original file line number Diff line number Diff line change
@@ -1,11 +1,5 @@
# Notes for contributors

1. Python hooks are supported now too. All you have to do is:
1. add a line to the `console_scripts` array in `entry_points` in `setup.py`
2. Put your python script in the `pre_commit_hooks` folder

Enjoy the clean, valid, and documented code!

* [Run and debug hooks locally](#run-and-debug-hooks-locally)
* [Run hook performance test](#run-hook-performance-test)
* [Run via BASH](#run-via-bash)
Expand All @@ -18,6 +12,7 @@ Enjoy the clean, valid, and documented code!
* [Prepare basic documentation](#prepare-basic-documentation)
* [Add code](#add-code)
* [Finish with the documentation](#finish-with-the-documentation)
* [Work with Python hooks](#work-with-python-hooks)

## Run and debug hooks locally

Expand Down Expand Up @@ -152,5 +147,38 @@ You can use [this PR](https://github.com/antonbabenko/pre-commit-terraform/pull/

### Finish with the documentation

1. Add hook description to [Available Hooks](../README.md#available-hooks).
1. Add the hook description to [Available Hooks](../README.md#available-hooks).
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.


1. [Install `tox`](https://tox.wiki/en/stable/installation.html)
2. To run tests, run:

```bash
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.


3. Before committing any changes (if you do not have `pre-commit` installed locally), run:

```bash
tox r -qq -e pre-commit
```

Make sure that all checks pass.

4. (Optional): If you want to see more details on MyPy checks, you can run:
MaxymVlasov marked this conversation as resolved.
Show resolved Hide resolved

```bash
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.


5. (Optional): You can find all available `tox` environments by running:

```bash
tox list
```
3 changes: 3 additions & 0 deletions .github/workflows/reusable-tox.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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.

- name: >-
Run tox envs: `${{ env.TOXENV }}`
id: tox-run
Expand All @@ -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

- name: Produce markdown test summary from JUnit
if: >-
!cancelled()
Expand Down
2 changes: 1 addition & 1 deletion pytest.ini
Original file line number Diff line number Diff line change
Expand Up @@ -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

filterwarnings =
error

Expand Down
14 changes: 13 additions & 1 deletion tox.ini
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,10 @@ warnings-to-errors = -Werror
description = Run pytest under {envpython}
dependency_groups =
testing

# In:
# 'tox run -e py -- --lf', 'tox run -- --lf', 'tox run -e py313,py312 -- --lf'
# '{posargs}' (positional arguments) == '--lf'
commands =
{envpython} \
{[python-cli-options]byte-errors} \
Expand All @@ -23,6 +27,9 @@ commands =
{tty:--color=yes} \
{posargs:--cov-report=html:{envtmpdir}{/}htmlcov{/}}
commands_post =
# Create GHA Job Summary markdown table of the coverage report
# https://github.blog/news-insights/product-news/supercharging-github-actions-with-job-summaries/
# '-' ignores non-zero exit code
MaxymVlasov marked this conversation as resolved.
Show resolved Hide resolved
-{envpython} \
{[python-cli-options]byte-errors} \
{[python-cli-options]max-isolation} \
Expand All @@ -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

{envpython} \
{[python-cli-options]byte-errors} \
{[python-cli-options]max-isolation} \
Expand Down Expand Up @@ -162,7 +170,9 @@ commands =
commands_post =
package = skip


# In:
# 'tox run -e pre-commit -- mypy-py313 --all'
# '{posargs}' == 'mypy-py313 --all'
[testenv:pre-commit]
description =
Run the quality checks under {basepython}; run as
Expand All @@ -183,6 +193,7 @@ commands =
{posargs:--all-files}

# Print out the advice on how to install pre-commit from this env into Git:
# '-' ignores non-zero exit code
MaxymVlasov marked this conversation as resolved.
Show resolved Hide resolved
-{envpython} \
{[python-cli-options]byte-errors} \
{[python-cli-options]max-isolation} \
Expand Down Expand Up @@ -216,6 +227,7 @@ commands_post =
); \
print("codecov-flags=MyPy", file=gh_output_fd); \
gh_output_fd.close()'
#
MaxymVlasov marked this conversation as resolved.
Show resolved Hide resolved
{envpython} \
{[python-cli-options]byte-errors} \
{[python-cli-options]max-isolation} \
Expand Down
Loading