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

Preserve symlinks #5266

Open
Globegitter opened this issue May 15, 2019 · 12 comments
Open

Preserve symlinks #5266

Globegitter opened this issue May 15, 2019 · 12 comments
Labels
topic: collection related to the collection phase type: enhancement new feature or API change, should be merged into features branch

Comments

@Globegitter
Copy link

It seems over the time pytest introduced realpath calls to resolve symlinks. When trying to use pytest with https://bazel.build/ however that falls flat because is executing all tests in an isolated sandbox (think lightweight docker) and is for performance reason just symlinking files rather than copying them and it expects tools to respect symlinks. Further it only puts the files into the sandbox that it needs for that test, so it might just be the pytest dependency, the testfile itself and the library that is needed for it. The problem I am now facing is, that pytest is resolving the symlink of the testfile, ending up in my real project directory, discovering a __init__.py file there and then complaining about missing imports because the isolated python provided by bazel does not have access to these for running the test.

It would be nice to get an option for pytest to just treat symlinks as the real files and not try to resolve them. In the nodejs world for example the flag --preserve-symlinks exists for that and it would be great to get the same for pytest.

Let me know if you need a minimal reproduction case, then I am happy to provide one.

pytest version 4.4.2, OS: Elementary OS 5 (Ubuntu 18.04)

@blueyed
Copy link
Contributor

blueyed commented May 15, 2019

--preserve-symlinks exists for that

Sounds sensible.

Is there another way that wouldn't require a flag?

IIRC I've started with that to make pytest tests/… work with tests being a symlink to project/foo/tests.
After all full resolving is not needed here maybe, but only for tests etc.

A minimal example would be great to have.

@Globegitter
Copy link
Author

Globegitter commented May 15, 2019

The way bazel executes pytest in our case is by passing it all the filepaths relative from the working directory, so e.g. pytest path/to/test.py or if there would be multiple ones pytest path/to/test1.py path/to/test2.py and path/to/test.py etc are the symlinked files. So possibly symlinks could be preserved only in the case of passing it direct (relative) file paths. WDYT?

But yeah will work on a repro asap.

@Globegitter
Copy link
Author

Globegitter commented May 15, 2019

Here is a minimal example not involving bazel: https://github.com/Globegitter/pytest-repro

@blueyed
Copy link
Contributor

blueyed commented May 15, 2019

So possibly symlinks could be preserved only in the case of passing it direct (relative) file paths. WDYT?

Might be a good compromise.

But it is also about internal lookup of contests etc (from parent dirs), and that would then mean to still resolve them internally at least in those places.
IIRC I've tried to adhere to this back then.

It happens here:

parts[0] = path.realpath()

It was done for #4325 via #4337.

blueyed added a commit to blueyed/pytest that referenced this issue May 15, 2019
@blueyed
Copy link
Contributor

blueyed commented May 15, 2019

Started working on it in #5270 (8 failures IIRC).
Moved your repro into a test: https://github.com/pytest-dev/pytest/pull/5270/files#diff-f31e02762db03e39bb7fe5f9fb52eb44 - does it match your expectations?

Some question came up already: how should nodes be reported? Using the symlink or target? (

result.stdout.fnmatch_lines(["real.py::test_nodeid PASSED*", "*1 passed in*"])
)

@blueyed blueyed added topic: collection related to the collection phase type: enhancement new feature or API change, should be merged into features branch labels May 15, 2019
@Globegitter
Copy link
Author

Thanks for the quick progress! Just tested it out and it does indeed work as expected - great. When you say "how should nodes be reported", do you mean which path it should report, when it prints to stdout?
E.g.

collected 1 item

path/to/symlink/test.py .                                    [100%]

I think in this case either or is fine, but I would prefer the symlink path, the behaviour it seems to be now as I can tell. That would imo also fit the behaviour of "preserve symlinks".

@Globegitter
Copy link
Author

@blueyed is there anything I can do to push this forward? Happy to also put in some time myself if I know what the best way forward is.

@blueyed
Copy link
Contributor

blueyed commented May 22, 2019

@Globegitter

do you mean which path it should report, when it prints to stdout?

Yes - basically how the nodeid should look like (that gets used internally also, including for test selection).
I agree that having the symlink there is more useful.

You could pick up my branch and make the failing tests work there, I assume that the resolving has to be done in a more inner place now - possibly only temporarily.

@blueyed
Copy link
Contributor

blueyed commented Oct 16, 2019

@Globegitter
Any chance you had worked on / looked into it?

@chaokunyang
Copy link

I came across the same problem.
For example, when I execute pytest ray/streaming/test/streaming_cluster_test.py, the module name changed from ray.streaming.test.streaming_cluster_test to python.test.streaming_cluster_test. Because I symlinked ray/streaming/test/ to python/streaming/test.
Would this be fixed or is that I use pytest in wrong way? I must use symlinks. So if this won't be fixed, I'll have to write w script to import my code and test it, which will be less good

@aditya-qure
Copy link

Any updates on this? I am getting 0% coverage.

Thanks

@nicoddemus
Copy link
Member

We have since then changed pytest and I think we do respect symlinks now: we no longer use Path.resolve(), using Path.absolute() instead when needed... there might be someplace where this is still an issue thought that we missed.

@aditya-qure can you post a MWE?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: collection related to the collection phase type: enhancement new feature or API change, should be merged into features branch
Projects
None yet
Development

No branches or pull requests

5 participants