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

add support for printing the diff of AST trees when running tests #3902

Merged
merged 6 commits into from
Sep 28, 2023

Conversation

jakkdl
Copy link
Contributor

@jakkdl jakkdl commented Sep 25, 2023

Description

As I've been working more on Black, I found the full printing of AST trees to be cumbersome and insanely long, but it regardless often containing necessary info, so I ended up modifying the value of that variable a lot and/or temporarily splitting tests into shorter files. But realized that diff-printing would solve that issue, and while not as simple as I'd initially hoped it wasn't too tricky to work out and is turning out very useful.

Checklist - did you ...

  • Add an entry in CHANGES.md if necessary?
    I don't think changes to test QoL warrants entries in CHANGES.md
  • Add / update tests if necessary?
    Given the precedence of test_assertFormatEqual I should probably write one.
  • Add new / update outdated documentation?
    "SKIP_AST_PRINT" isn't documented anywhere, but it and this should probably be in CONTRIBUTING.md and/or the contributing documentation on RTD - it took me a while to find myself.

@JelleZijlstra JelleZijlstra added the skip news Pull requests that don't need a changelog entry. label Sep 25, 2023
@JelleZijlstra
Copy link
Collaborator

This is a good, idea, thanks! The full AST is indeed insanely long. A few thoughts:

  • Maybe we should just remove the environment variables and make this the only option during tests? Developers who really want the full AST can always modify the code, but hidden env vars aren't a great user experience in any case.
  • "AST" is really a misnomer here; lib2to3 produces a CST, not an AST.

@jakkdl
Copy link
Contributor Author

jakkdl commented Sep 25, 2023

Maybe we should just remove the environment variables and make this the only option during tests? Developers who really want the full AST can always modify the code, but hidden env vars aren't a great user experience in any case.

I just realized, this would be much easier to use if added as a CLI option in tests/conftest.py. That way we also get visibility in pytest --help for free.
I think keeping ability to print the full tree is still valuable, but we could probably default that to off, and default tree-diff-printing to on.

"AST" is really a misnomer here; lib2to3 produces a CST, not an AST.

yeah, let's rename them at the same time.

@github-actions
Copy link

github-actions bot commented Sep 25, 2023

diff-shades reports zero changes comparing this PR (4bf33c9) to main (3dcacdd).


What is this? | Workflow run | diff-shades documentation

…t variable to a pytest option, add test and update old test to work with patching, remove py37 from tox default envlist
Copy link
Collaborator

@JelleZijlstra JelleZijlstra left a comment

Choose a reason for hiding this comment

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

Thanks! Left some comments mostly on wording.

docs/contributing/the_basics.md Outdated Show resolved Hide resolved
docs/contributing/the_basics.md Outdated Show resolved Hide resolved
docs/contributing/the_basics.md Outdated Show resolved Hide resolved
tests/conftest.py Outdated Show resolved Hide resolved
tests/conftest.py Outdated Show resolved Hide resolved
tests/util.py Outdated
ast_print = not os.environ.get("SKIP_AST_PRINT")
ast_print_diff = not os.environ.get("SKIP_AST_PRINT_DIFF")
if actual != expected and (ast_print or ast_print_diff):
# need to import inside the function for the monkeypatching tests to work
Copy link
Collaborator

Choose a reason for hiding this comment

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

Another approach which should work and I think is cleaner is to do from . import conftest in the global scope, and then use the qualified names like conftest.PRINT_FULL_TREE here.

@jakkdl
Copy link
Contributor Author

jakkdl commented Sep 28, 2023

The CI fail seem unrelated to this PR

@JelleZijlstra JelleZijlstra merged commit 9b82120 into psf:main Sep 28, 2023
35 of 41 checks passed
@jakkdl jakkdl deleted the ast_tree_diff branch September 28, 2023 16:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip news Pull requests that don't need a changelog entry.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants