Copy py.code code to py.test

[nicoddemus]

@hpk42 @RonnyPfannschmidt, few things to discuss still:

• py.code has a lot of public members which are well documented. To move the documentation over to pytest we should also make those classes public IMO. Should they live in the pytest namespace, or some other namespace like pytest.code?
• This branch is pulling py from my repository, with py.code removed; all tests for py are passing on that repository, I will open a PR for py later once we agree on how the docs should be moved.
• With this move, py should be bumped to 2.0. Unfortunately all pytest versions out there have the spec py>=1.4.29 (or something), which would cause them to try to use 2.0 when installing an older version. Not sure how to solve this.

(There's a single test failing on pypy on Travis, I will investigate this later)

Fix #103

[RonnyPfannschmidt]

We should strongly investigate trimming that api down before making it public under pytest

There should not be a pylib 2.0 we should consider pylib eol

[hpk42]

Bruno, thanks for tackling this!

i think it's a good idea to first move to pytest.code without changing much. I would, however, not document or expose pytest.code -- maybe it's even better to name it pytest._code therefore.

I'd also not remove py.code -- this breaks too much code as you already noted.

IOW i think we can just copy py.code to pytest._code and treat it under new ownership there, with us feeling free to remove, streamline things as we go. this way, pytest-2.9 will "just work" and nothing will break which, given the huge installation numbers, is a better plan than conciously breaking things. If possible, removing some unused things from pytest._code after the initial merge is also a good idea so we don't start with too much cruft. Also deleting code is great :)

We could of course still decide that we offer some public pytest.code at some point later.

[RonnyPfannschmidt]

does anyone have any idea why pypy fails?

[nicoddemus]

I didn't have a chance to look it up yet. The failing test is from py.code, so I will install pypy and see if the original py.code test had the same problem or not.

[nicoddemus]

I will only have time to look it up next week though, I think.

[nicoddemus]

Took another dab at this. 😅

• Rebased into the latest features branch;
• Moved py.code as _pytest._code;
• I added the docs for ExceptionInfo into pytest.raises itself, because it is the only point where that part of the py.code API is exposed to the user.
• Dropped _pytest._code.assertionnew and _pytest._code.assertionold since they are superseded by pytest's own assertion modules.

ExceptionInfo

One thing I realized is that because of ExceptionInfo, code in 2.9 might still break:

def test_foo():
    with pytest.raises(RuntimeError) as exc_info:
        foo()
    assert isinstance(exc_info, py.code.ExceptionInfo)  # <- works in 2.8, will break in 2.9

And the worst part is that the user would have to explicitly now import from _pytest._code.ExceptionInfo. I agree that this example is probably a little rare, but nonetheless we should be aware of this problem.

Since ExceptionInfo is the only py.code explicitly mentioned in the docs, how about we expose it officially in the pytest namespace, as pytest.ExceptionInfo? This way we don't expose the entire _pytest._code module, and provide a nice public API for that object.

py.code slipping unnoticed and wrecking havoc

Since we are not dropping py.code from pylib, I fear it will be easy for code which uses py.code to slip into features from master, or even in the future if we don't notice while reviewing a new contribution. The problem is that most likely no tests will break since we probably will keep the API compatible, but client code might break in subtle ways (a code path from pytest suddenly returning a py.code.ExceptionInfo instead of _pytest._code.ExceptionInfo, for example).

I give it some thought I think we can solve that by writing a small flakes8 extension that checks for code accessing py.code, and add that extension to the flakes testenv in tox.ini.

Another idea I had was to block py.code from being imported when executing the pytest test suite by putting some code like this in conftest.py:

py.code  # lazy creation
del py.code
del sys.modules['py.code']

This seems to work well, but I fear that by the time the code executes other pytest modules have already been imported, and might already been holding references to functions or methods in py.code.

What do you guys think?

[nicoddemus]

I tested py with pypy locally and the same test that failed on Travis fails on my Windows computer, so it seems the failure already exists in the current default branch.

[RonnyPfannschmidt]

since we don't have a travis for pylib i suspect the bug has been there since a while,
please xfail it in a separate commit

wrt selftesting,

maybe we should as a long term goal ensure, that no import in pytest at the module level triggers a import of pylib, so we can kill the reliance on py.path and pathlib

and we might want a local selftest python script, which does the sys.path mangleing before importing and running pytest.main

[nicoddemus]

Marked the test_comments as xfail as suggested by @RonnyPfannschmidt and now everything is green.

and we might want a local selftest python script, which does the sys.path mangleing before importing and running pytest.main

I like this idea, but unfortunately it does not work, there are other parts in py that depend on py.code so disabling might break other parts (for example py.path).

I created a simple script with:

if __name__ == '__main__':
    # disable py.code from being importable
    import sys
    import py.code
    del py.code
    del sys.modules['py.code']
    import pytest
    raise SystemExit(pytest.main())

But using it to run the test suite I get errors like:

Traceback (most recent call last):
  File "app_main.py", line 75, in run_toplevel
  File "d:\Programming\pypy\lib-python\2.7\atexit.py", line 24, in _run_exitfuncs
    func(*targs, **kargs)
  File "x:\pytest\.envpypy\site-packages\py\_path\local.py", line 841, in try_remove_lockfile
    lockfile.remove()
  File "x:\pytest\.envpypy\site-packages\py\_path\local.py", line 201, in remove
    if self.check(dir=1, link=0):
  File "x:\pytest\.envpypy\site-packages\py\_path\local.py", line 371, in check
    return super(LocalPath, self).check(**kw)
  File "x:\pytest\.envpypy\site-packages\py\_path\common.py", line 190, in check
    return self.Checkers(self)._evaluate(kw)
  File "x:\pytest\.envpypy\site-packages\py\_path\common.py", line 64, in _evaluate
    if py.code.getrawcode(meth).co_argcount > 1:
  File "x:\pytest\.envpypy\site-packages\py\_apipkg.py", line 123, in __makeattr
    raise AttributeError(name)

I think static checking might be the only option to enforce py.code is not used within pytest. Any other ideas?

[RonnyPfannschmidt]

I wonder if the only problem source is py.path
#1260 might reallow the sys.path mangling

[nicoddemus]

According to my py patch at 89dae997, in warning.py, common.py and forkedfunc.py.

Edit: warning and common only depend on getrawcode, forkedfunc depends on py.code.ExceptionInfo.

[nicoddemus]

Just to be clear, in my fork I deleted py.code, so we can look there to see what we would have to change in py so other parts of the code don't depend on py.code anymore... but even then it's no guarantee that in the future some code change reintroduces this dependency by mistake.

At this point I wonder if it is worth the trouble trying to do the sys.path mangling at all.

[RonnyPfannschmidt]

Let's make that a part of review, new code that adds py.code usage should get extra scrutiny

[nicoddemus]

Seems reasonable. We can always introduce static checking later if we deem necessary.

About where ExeptionInfo is implemented, we can keep it an implementation detail, that's how it looks now.

Any other comments? @hpk42?

[nicoddemus]

Hi guys, anything else should work here? I would like to continue work on any pending tasks for this issue, if we still have any.

[RonnyPfannschmidt]

we should ensure that the exposed api for this shows a pending deprecation warning various places, so we can work at replacing the exceptioninfo objects alltogther

[nicoddemus]

Hey @RonnyPfannschmidt and @hpk42,

Just rebased this into the features branch. Do you guys think this can be merged?

[RonnyPfannschmidt]

i think it needs a more visible note about the move
and we should consider marking the api as unstable

all in all well done, thanks :)

[The-Compiler]

It's in pytest._code anyways (i.e. non-public), so I don't think it needs to be marked as unstable, as it's private API 😉

[RonnyPfannschmidt]

we do have some object exposure points that are unfortunate (raises and traceback cutting/report information)

[nicoddemus]

I increased the importance of the node like @RonnyPfannschmidt asked and moved it to the top to gain more visibility.

[nicoddemus]

For your convenience, you can see it rendered here.

[RonnyPfannschmidt]

Well done, thanks for the hard work