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

Make node ID modification more stable. #1118

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

criemen
Copy link
Contributor

@criemen criemen commented Aug 21, 2024

We force test collection on the controller node, as we run some global setup there before starting the workers. That setup also depends on the test node IDs, but currently, pytest-xdist only modifies the test node IDs on the workers, that've been started and where setup_config has been called. I don't see a good reason for this, and for creating a separate config entry, so let's just check the right condition in the pytest hook. That way, the hook will always do the same node ID transformation, regardless of pytest-xdist setup.

Thanks for submitting a PR, your contribution is really appreciated!

Here's a quick checklist that should be present in PRs:

  • Make sure to include reasonable tests for your change if necessary

  • We use towncrier for changelog management, so please add a news file into the changelog folder following these guidelines:

    • Name it $issue_id.$type for example 588.bugfix;

    • If you don't have an issue_id change it to the PR id after creating it

    • Ensure type is one of removal, feature, bugfix, vendor, doc or trivial

    • Make sure to use full sentences with correct case and punctuation, for example:

      Fix issue with non-ascii contents in doctest text files.
      

We force test collection on the controller node, as we run some global
setup there before starting the workers. That setup also depends on the
test node IDs, but currently, pytest-xdist only modifies the test node IDs
on the workers, that've been started and where `setup_config` has been called.
I don't see a good reason for this, and for creating a separate config entry,
so let's just check the right condition in the pytest hook.
That way, the hook will always do the same node ID transformation, regardless
of pytest-xdist setup.
@RonnyPfannschmidt
Copy link
Member

Node id modification needs to be dropped all together

@criemen
Copy link
Contributor Author

criemen commented Aug 21, 2024

That is a good point - as all the unit tests fail on this PR anyways, I'll see what I can do.

@RonnyPfannschmidt
Copy link
Member

Doing the stuff the hack with node id enabled in a more natural way is a major undertaking

Only got there as a follow-up

@criemen
Copy link
Contributor Author

criemen commented Aug 21, 2024

I'll collect feedback on #1119 and if that's not a viable route, I'll try to resurrect this PR instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants