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

Highlight the path of file location in error report #1778

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
4 participants
@suzaku
Copy link
Contributor

suzaku commented Jul 30, 2016

Make it more obvious when we need to copy the file path.

@suzaku suzaku force-pushed the suzaku:highlight-file-loc branch 2 times, most recently from af3b9b0 to 57704de Jul 30, 2016

Highlight the path of file location in error report
    So that it's more obvious when we need to copy the file path.

@suzaku suzaku force-pushed the suzaku:highlight-file-loc branch from 57704de to 4c53909 Jul 30, 2016

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jul 30, 2016

Coverage Status

Coverage increased (+0.0009%) to 92.317% when pulling 4c53909 on suzaku:highlight-file-loc into ffb583a on pytest-dev:master.

@nicoddemus

This comment has been minimized.

Copy link
Member

nicoddemus commented Jul 30, 2016

Thanks for the PR! 😄

Here's what it looks like:

pytest-highlight-file

This looks good to me, if nobody else has any comments I will merge this tomorrow. 👍

@The-Compiler

This comment has been minimized.

Copy link
Member

The-Compiler commented Jul 30, 2016

Thanks for the screenshot @nicoddemus! Shouldn't this go to features though?

@nicoddemus

This comment has been minimized.

Copy link
Member

nicoddemus commented Jul 30, 2016

Good point, I could rebase it tomorrow before merging though.

Weird that the freeze test env is failing on Windows, it was passing just a few days ago:

freeze runtests: commands[1] | C:\projects\pytest\.tox\freeze\Scripts\python tox_run.py
Traceback (most recent call last):
  File "site-packages\PyInstaller\loader\rthooks\pyi_rth_pkgres.py", line 11, in <module>
  File "<frozen importlib._bootstrap>", line 969, in _find_and_load
  File "<frozen importlib._bootstrap>", line 958, in _find_and_load_unlocked
  File "<frozen importlib._bootstrap>", line 664, in _load_unlocked
  File "<frozen importlib._bootstrap>", line 634, in _load_backward_compatible
  File "c:\projects\pytest\.tox\freeze\lib\site-packages\PyInstaller\loader\pyimod03_importers.py", line 389, in load_module
    exec(bytecode, module.__dict__)
  File "site-packages\pkg_resources\__init__.py", line 2958, in <module>
  File "site-packages\pkg_resources\__init__.py", line 2944, in _call_aside
  File "site-packages\pkg_resources\__init__.py", line 2986, in _initialize_master_working_set
UnboundLocalError: local variable 'dist' referenced before assignment
Failed to execute script pyi_rth_pkgres
ERROR: InvocationError: 'C:\\projects\\pytest\\.tox\\freeze\\Scripts\\python tox_run.py'

Seems to be related to executing the pyi_rth_pkgres internal module from PyInstaller. Perhaps @codewarrior0 or @htgoebel have any hints at what might be the problem?

@The-Compiler

This comment has been minimized.

Copy link
Member

The-Compiler commented Jul 30, 2016

That's a setuptools bug, see pypa/setuptools#702 - pypa/setuptools#703 was now merged, and seeing how often setuptools is released I bet there will be a new release very soon.

@nicoddemus

This comment has been minimized.

Copy link
Member

nicoddemus commented Jul 30, 2016

Great, thanks for looking it up! 😁

@suzaku

This comment has been minimized.

Copy link
Contributor Author

suzaku commented Jul 30, 2016

Thanks for replying. Let me know if there's anything I can do to fix the failing build.

@@ -755,14 +764,18 @@ def f():
assert tw.lines[0] == " def f():"
assert tw.lines[1] == "> g(3)"
assert tw.lines[2] == ""
assert tw.lines[3].endswith("mod.py:5: ")

This comment has been minimized.

Copy link
@nicoddemus

nicoddemus Jul 30, 2016

Member

@suzaku just a question, why did you changed those tests? Your change to the terminal writer required those to be changed?

This comment has been minimized.

Copy link
@suzaku

suzaku Jul 31, 2016

Author Contributor

@nicoddemus Yes, otherwise the tests would fail. After I used write to print out the highlighted file path, the tests complained that there's no write method on the TWMock object. So I added a write method for TWMock and the corresponding way to check writes.

I think there must be a better way to do this, maybe I can refactor these tests and eliminate replace TWMock with a mock.Mock later in another pull request?

@nicoddemus

This comment has been minimized.

Copy link
Member

nicoddemus commented Aug 1, 2016

Closing in favor of #1781. Thanks again @suzaku!

@nicoddemus nicoddemus closed this Aug 1, 2016

@suzaku

This comment has been minimized.

Copy link
Contributor Author

suzaku commented Aug 1, 2016

:)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.