Skip to content

Change how nodeids are created in relation to rootdir #11245

Open

Description

@bluetech

This issue is about the path part of the nodeid (the part before the ::).

Problem

Generally, the path part of a nodeid is created by taking the node's path relative to the rootdir.
But, if the node's path is not below the rootdir ("out of tree"), then we create its nodeid relative to the initial paths (= paths given in the command line), specifically the first one which contains the path. This was added in 14b6380 as a fix for an issue with out-of-tree --pyargs (2775) which would result in an empty nodeid path.

This change broke an important property of the nodeid, that it is always relative to the rootdir, or said differently, that the nodeid cannot be relied upon to be the root of all nodeids. This causes various issues that crop up when such invariants are broken:

  • ambiguous nodeids
  • duplicate nodeids
  • incorrect printing of nodeids

See #11186, #6605, #3714.

The issues mostly stem from the fact that a nodeid does not carry around the context of which path it it relative to, so the context is lost and becomes ambiguous.

Proposed solution

Perhaps the simplest fix is to change back to always using the rootdir, but now allowing .. (parent directory) segments in the nodeid path. This is kind of ugly, but running tests outside the rootdir is discouraged anyway, so I'm not too worried about it, in fact it might encourage settings the rootdir correctly...

I have an initial implementation here: https://github.com/bluetech/pytest/commits/rootdir-rel

Other solutions?

Another solution might be to change nodeids to be structured and maintain the context. However, users still interact with nodeids as strings so I don't think this can go very far.

And another solution can be that out-of-tree nodeids just always use absolute paths e.g. /my/out/of/tree/test.py::test_it instead of ../../of/tree/test.py::test_it. Maybe it's better?

Activity

  1. added
    type: proposalproposal for a new feature, often to gather opinions or design the API around the new feature
    on Jul 23, 2023
  2. RonnyPfannschmidt commented on Jul 26, 2023

    @RonnyPfannschmidt
    Member

    i'd like ot see some type of prefix for those nodeids

    for example

    site!my/import/name::...
    rootdir/relative/test_example.py::...
    ./cwd/relative/test_example.py
    

    pytest can then choose to output
    rootidr relative, invocation dir relative/cwd or prefixed for out of tree imports

    this relates to my draft proposal for #10824 which is supposed to enable specifying import roots for parts of the testsuite and it might help to make that easier

  3. nicoddemus commented on Jul 26, 2023

    @nicoddemus
    Member

    Perhaps the simplest fix is to change back to always using the rootdir, but now allowing .. (parent directory) segments in the nodeid path

    I agree, this seems simple enough, intuitive, and also a step that enables us to move further for further node id changes.

  4. eleanorjboyd commented on Aug 2, 2023

    @eleanorjboyd

    Hello! Jumping in as I filed issue #11235 which was related. My main concern which I am curious if this PR / investigation could fix is about the lack of circularity in pytest node ids. Given the following:

    file structure:

    directory
    --tests
    ----test_example.py
    ------test_function (function in test_example.py)
    ----pytest.ini
    --simple_plugin
    ----__init__.py
    

    Lets say I have a simple plugin that only prints out the testIds.

    if I run pytest -p simple_plugin -c tests/pytest.ini from the directory node my output from the plugin (ie the testids) would be
    test_example.py::test_function

    if I then want to take this id and run this test I cannot do so given my current location / pytest args since pytest -p simple_plugin -c tests/pytest.ini test_example.py::test_function returns that it cannot find test_example.py::test_function.

    By "circularity" I mean that a testid returned from one run, cannot be used in the same location with the same args to call the same test. Instead, the test id or the invocation directory must be changed since the id is not recognized.

    It seems that your PR only handles tests not under the rootdir which means this example I don't think would apply. Please let me know if I am misunderstanding something but wanted to make sure this point was also placed in this discussion as it seems odd that testids cannot be gathered then run in the example above.

    My thought in terms of a solution is just that test ids provided as arguments should be compared against the rootdir (which in this case is changed by the -c arg) therefore inputted testids are also evaluated relative to the rootdir

    Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    topic: collectionrelated to the collection phasetype: proposalproposal for a new feature, often to gather opinions or design the API around the new feature

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions