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

[RFC/WIP]: assert: use safeformat for display with -vv #6020

Closed
wants to merge 5 commits into from

Conversation

blueyed
Copy link
Contributor

@blueyed blueyed commented Oct 21, 2019

TODO:

  • real plugin hook?
  • should handle other places where safeformat is used also, via
    args/options to the hook then likely.
  • test

Ref: #3962
Ref: #5933

TODO:

- [ ] real plugin hook?
- [ ] should handle other places where safeformat is used also, via
      args/options to the hook then likely.

Ref: pytest-dev#3962
Ref: pytest-dev#5933
@blueyed blueyed added the topic: rewrite related to the assertion rewrite mechanism label Oct 21, 2019
@blueyed
Copy link
Contributor Author

blueyed commented Oct 21, 2019

Keeping newlines can result in many lines:

testing/test_pdb.py:358: in test_meta_different_init_signture
    assert "set_trace" in sys._getframe().f_code.co_names
E   assert 'set_trace' in ('pdb',
E      'Pdb',
E      'PdbTest',
E      'sys',
E      '_getframe',
E      'f_code',
E      'co_names',
E      '_pytest.warning_types',
E      'PytestAssertRewriteWarning',
E      'warnings',
E      'warn_explicit',
E      '@pytest_ar',
E      '_call_reprcompare',
E      '_call_reprdisplay',
E      '@py_builtins',
E      'locals',
E      '_should_repr_global_name',
E      'AssertionError',
E      '_format_explanation',
E      'local',
E      'GLOBAL_PDB',
E      '_pdb2',
E      'stdout')
E    +  where ('pdb',
E        'Pdb',
E        'PdbTest',
E        'sys',
E        '_getframe',
E        'f_code',
E        'co_names',
E        '_pytest.warning_types',
E        'PytestAssertRewriteWarning',
E        'warnings',
E        'warn_explicit',
E        '@pytest_ar',
E        '_call_reprcompare',
E        '_call_reprdisplay',
E        '@py_builtins',
E        'locals',
E        '_should_repr_global_name',
E        'AssertionError',
E        '_format_explanation',
E        'local',
E        'GLOBAL_PDB',
E        '_pdb2',
E        'stdout') = <code object test_meta_different_init_signture at 0x7fcaa73a4b70, file "…/src/pdbpp/testing/test_pdb.py", line 328>.co_names
E    +    where <code object test_meta_different_init_signture at 0x7fcaa73a4b70, file "…/src/pdbpp/testing/test_pdb.py", line 328> = <frame at 0x562badb85620, file '…/src/pdbpp/testing/test_pdb.py', line 358, code test_meta_different_init_signture>.f_code
E    +      where <frame at 0x562badb85620, file '…/src/pdbpp/testing/test_pdb.py', line 358, code test_meta_different_init_signture> = <built-in function _getframe>()
E    +        where <built-in function _getframe> = sys._getframe

Alternatively we could bump the width to something very big, so that it wraps the automatically (hopefully), or replace newlines from pformat with spaces.

if util._reprdisplay is not None:
return util._reprdisplay(obj)
# XXX: happens/used with testing/test_assertion.py::TestImportHookInstallation::test_rewrite_assertions_pytester_plugin # noqa: E501
return _saferepr(obj)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@blueyed blueyed changed the title WIP: assert: use safeformat for display with -vv [RFC/WIP]: assert: use safeformat for display with -vv Oct 21, 2019
@nicoddemus
Copy link
Member

The change looks good, I would avoid adding a hook until we have real use cases for the hook other than the builtin implementation.

blueyed added a commit to blueyed/pytest-django that referenced this pull request Nov 1, 2019
pytest fails to handle `pytest.fail.Exception` when raised in `repr`.

This changes pytest-django to raise `RuntimeError` instead.

Ref: pytest-dev/pytest#6020
Fixes pytest-dev#713
@blueyed
Copy link
Contributor Author

blueyed commented Nov 5, 2019

The change looks good, I would avoid adding a hook until we have real use cases for the hook other than the builtin implementation.

Agreed.
could we have this now then already?
(given that no existing tests are failing we're kind of in the "good area" still)

@nicoddemus
Copy link
Member

could we have this now then already?

Yep, just needs at least a test and CHANGELOG. 👍

@blueyed blueyed closed this Nov 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: rewrite related to the assertion rewrite mechanism
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants