pytest-dev / py

Python development support library (note: maintenance only)
MIT License
69 stars 105 forks source link

use py.builtin._basestring for py2/3 compatibility #86

Closed montefra closed 8 years ago

montefra commented 8 years ago

when visiting a path, check that the input values are a string in a python2/3 compatible way. See issue #81

RonnyPfannschmidt commented 8 years ago

good work, for avoiding regressions, can you perhaps put the type variation into the tests for the feature as well?

montefra commented 8 years ago

@RonnyPfannschmidt : this should do.

I'll think if there is an easy way to test it. I don't think that importing the __future__ is ok. Probably I need something using pytest.mark.parametrize and skipif.

Do I have to add some changelog or similary anywhere else?

Edit:

I was writing when you replied. I'll do it soon.

montefra commented 8 years ago

I did try to run tox but I got E.g.

py34 create: /data01/montefra/Code/Gitrepos/py/.tox/py34
py34 installdeps: pytest~=2.9.0
py34 inst: /data01/montefra/Code/Gitrepos/py/.tox/dist/py-1.4.32.dev1.zip
py34 installed: py==1.4.32.dev1,pytest==2.9.2
py34 runtests: PYTHONHASHSEED='2583340150'
py34 runtests: commands[0] | py.test --confcutdir=.. -rfsxX --junitxml=/data01/montefra/Code/Gitrepos/py/.tox/py34/log/junit-py34.xml
Traceback (most recent call last):
  File "../.tox/py34/bin/py.test", line 7, in 
    from pytest import main
  File "/data01/montefra/Code/Gitrepos/py/.tox/py34/lib/python3.4/site-packages/pytest.py", line 21, in 
    from _pytest.config import (
  File "/data01/montefra/Code/Gitrepos/py/.tox/py34/lib/python3.4/site-packages/_pytest/config.py", line 8, in 
    import py
ImportError: No module named 'py'
ERROR: InvocationError: '/data01/montefra/Code/Gitrepos/py/.tox/py34/bin/py.test --confcutdir=.. -rfsxX --junitxml=/data01/montefra/Code/Gitrepos/py/.tox/py34/log/junit-py34.xml'   

ps: I can't access the documentation: http://pylib.readthedocs.io/en/latest/ returns me:

Welcome to Read the Docs
This is an autogenerated index file.

Please create a /home/docs/checkouts/readthedocs.org/user_builds/pylib/checkouts/latest/doc/index.rst or /home/docs/checkouts/readthedocs.org/user_builds/pylib/checkouts/latest/doc/README.rst file with your own content.

If you want to use another markup, choose a different builder in your settings.
RonnyPfannschmidt commented 8 years ago

the docs issue is my fault, we will reconfigure readthedocs sometime today

montefra commented 8 years ago

on a side note: can I add python 3.5 in tox.ini?

RonnyPfannschmidt commented 8 years ago

of course, please do so, i forgot doing it after the move

montefra commented 8 years ago

I have added py35 environment. But I've noticed one thing that puzzles me testing/path/common.py is not run by tox, nor by Travis CI. Why is that? My changes are not run in the test suite. And I have no idea how to do it: I did go a bit through the fixtures and setups and I haven't understood much of it.

This is what I get:

-> tox -e py35          
GLOB sdist-make: /data/Codes/Gitrepos/py/setup.py
py35 inst-nodeps: /data/Codes/Gitrepos/py/.tox/dist/py-1.4.32.dev1.zip
py35 installed: py==1.4.32.dev1,pytest==2.9.2
py35 runtests: PYTHONHASHSEED='3938473908'
py35 runtests: commands[0] | py.test --confcutdir=.. -rfsxX --junitxml=/data/Codes/Gitrepos/py/.tox/py35/log/junit-py35.xml
========================================================================= test session starts ==========================================================================
platform linux -- Python 3.5.1, pytest-2.9.2, py-1.4.32.dev1, pluggy-0.3.1
rootdir: /data/Codes/Gitrepos/py, inifile: tox.ini
collected 478 items / 4 skipped 

test_iniconfig.py ...........................................
code/test_assertion.py ...............ss.......
code/test_code.py ..............
code/test_excinfo.py ...........s........s....................................................
code/test_source.py .....................................................................
io_/test_capture.py ..................s.............s.............s................s.....................
io_/test_saferepr.py ........
io_/test_terminalwriter.py ............................................
log/test_log.py .................
log/test_warning.py x.....
path/test_cacheutil.py .............
process/test_cmdexec.py .....
process/test_forkedfunc.py ..............
process/test_killproc.py .
root/test_builtin.py ...................
root/test_error.py .....
root/test_py_imports.py ................
root/test_std.py ...
root/test_xmlgen.py ...................

----------------------------------------------- generated xml file: /data/Codes/Gitrepos/py/.tox/py35/log/junit-py35.xml -----------------------------------------------
======================================================================= short test summary info ========================================================================
SKIP [4] testing/io_/test_capture.py:236: text output different for bytes on python3
SKIP [4] /data/Codes/Gitrepos/py/testing/path/common.py:445: sys.version_info < (3,6)
SKIP [1] /data/Codes/Gitrepos/py/testing/code/test_excinfo.py:288: could not import 'jinja2'
SKIP [2] /data/Codes/Gitrepos/py/testing/code/test_assertion.py:170: could not import 'py._code._assertionold'
SKIP [1] /data/Codes/Gitrepos/py/testing/code/test_excinfo.py:183: could not import 'decorator'
XFAIL log/test_warning.py::test_forwarding_to_warnings_module
RonnyPfannschmidt commented 8 years ago

Good find, ill check

RonnyPfannschmidt commented 8 years ago

there was a mistake in a pr i merged previously causing all tests to be skipped

fortunately pytest 3.0 made me find it, and i'm cleaning things up now

montefra commented 8 years ago

@RonnyPfannschmidt : Good to know.

Let you know when you are done so I'll merge master into this branch and see if the tests run.

RonnyPfannschmidt commented 8 years ago

@montefra currently the needed changes are on #88 - since its large i'd like to have another pair of eyes from a core maintainer on it just in case i missed something

montefra commented 8 years ago

Thank for the work. I'll wait for it to land in master before proceeding.

RonnyPfannschmidt commented 8 years ago

@montefra sorry for the delay, the cleanups on the other branch started to break out of proportion i started #90 to stop blocking your good work and get something to the point done

RonnyPfannschmidt commented 8 years ago

@montefra the new run exposed a typo, also now your tests run as expected, thanks for the patience

montefra commented 8 years ago

Thank you. I'll work on this branch in the next couple of days.

montefra commented 8 years ago

@RonnyPfannschmidt : do you prefer me to merge or rebase this branch on master?

RonnyPfannschmidt commented 8 years ago

@montefra please rebase so we can keep the number of merges to a minimum

RonnyPfannschmidt commented 8 years ago

@montefra good work

interesting, any idea why did the byte string fail on python3?

montefra commented 8 years ago

I was about to write about this:

self = <py._path.common.Visitor object at 0x7f58774094a8>, path = svnwc('/tmp/pytest-of-susefra/pytest-17/test_make_repo0/path1wc/otherdir')

    def gen(self, path):
        try:
            entries = path.listdir()
        except self.ignore:
            return
        rec = self.rec
        dirs = self.optsort([p for p in entries
                    if p.check(dir=1) and (rec is None or rec(p))])
        if not self.breadthfirst:
            for subdir in dirs:
                for p in self.gen(subdir):
                    yield p
        for p in self.optsort(entries):
>           if self.fil is None or self.fil(p):
E           TypeError: 'bytes' object is not callable

Byte string are not base_string in python3, so isinstance(fil, py.builtin._basestring) is False. Does FNMatcher support byte strings? If yes should be easy to add byte strings.

montefra commented 8 years ago

You asked about non ascii character. Per se would be easy to add it somehow. I would need a new test, using tmpdir and ``tmpdir.ensure("some-string-with-non-ascii)" and then run visit('non-ascii').

Would be such a test be picked up automatically? I find the logic tests complicated (e.g. my new the tests in testing/path/test_local.py are clearly run, but the module itself is not present in the list from pytest and the three tests appears in three different positions, as the tests of 1d95912 appears to show)

RonnyPfannschmidt commented 8 years ago

@montefra yes, the structure used there is quite old, back then py.test had no fixtures and was part of pylib

RonnyPfannschmidt commented 8 years ago

@montefra such a test would be picked up

montefra commented 8 years ago

@RonnyPfannschmidt : I'll work on it later today, if I manage

RonnyPfannschmidt commented 8 years ago

@montefra as your change looks good, i'd like to merge, any objections?

montefra commented 8 years ago

If you don't need tests for non-ascii go ahead.