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

Parallelize pytest with pytest-xdist #1247

Merged
merged 1 commit into from
Jan 10, 2023

Conversation

alexrudd2
Copy link
Collaborator

Consider adding this requirement for pytest-xdist to speed up pytest runs by running them in
parallel
.

Pros:
+50% speedup on my laptop

Cons:

  • Extra dependency
  • Pytest output does not list individual tests any more
collected 417 items

test/test_all_messages.py ...                                                                              [  0%]
test/test_bit_read_messages.py .........                                                                   [  2%]
test/test_bit_write_messages.py .........                                                                  [  5%]
test/test_client.py ............................................s.........                                 [ 17%]
test/test_client_sync.py .....
gw0 [417] / gw1 [417] / gw2 [417] / gw3 [417] / gw4 [417] / gw5 [417] / gw6 [417] / gw7 [417]
.......................................s.................................................................. [ 25%]
.......................................................................................................... [ 50%]
.......................................................................................................... [ 76%]
...................................................................................................        [100%]

Copy link
Collaborator

@janiversen janiversen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure this is a good idea, at least you either:

  • need to exclude all tests that starts the server
  • assign different ports to different tests that open a listener.

@@ -47,7 +47,7 @@ jobs:
cmd: make -C doc/ html
type: lint
- name: pytest
cmd: pytest --cov=pymodbus --cov=test --cov-report=term-missing --cov-report=xml -v --full-trace --timeout=20
cmd: pytest --cov=pymodbus --cov=test --cov-report=term-missing --cov-report=xml -v --full-trace --timeout=20 --numprocesses auto
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this works it is pr accident. Multiple tests open a listener on port 5020, if they run in parallel they will fail.

@alexrudd2
Copy link
Collaborator Author

https://github.com/riptideio/pymodbus/blob/035ffc2991cceb109fc0243709e765a0b98a19a4/test/test_server_asyncio.py#L189-L199

Sorry for not understanding the full test suite. However, is the port not already randomized? If I change random_port to 5020 the test suite fails. As-is, it passes.

@alexrudd2 alexrudd2 requested a review from janiversen January 10, 2023 18:07
Copy link
Collaborator

@janiversen janiversen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not understand why this works, it should not but since it does...

@janiversen janiversen merged commit 7f994ed into pymodbus-dev:dev Jan 10, 2023
@janiversen
Copy link
Collaborator

random_port is a special test used only in test_server_asyncio.py, but that does not solve problem.

There are not much to understand about the test suite, a lot of the tests spin up a server which listens on a given port (typically 5020), then in the same test the client connects to port 5020 and do whatever the purpose of the test is.

Running several of these tests in parallel is impossible, because on 1 system only 1 process can listen to 1 port. Look at test_examples.py as an example.

@janiversen
Copy link
Collaborator

Merged, due to my lack of understanding why this works.

Thanks for the PRs, looking forward to see more PRs from your side.

@alexrudd2
Copy link
Collaborator Author

Merged, due to my lack of understanding why this works.

Thanks for the PRs, looking forward to see more PRs from your side.

I do not either 😂. My understanding of the expected problem matches yours. I was expecting to have to use @pytest.mark.paramaterize but it worked somehow...

Glad to be able to contribute in a small way. My employer maintains several FOSS packages dependant on pymodbus, and I ported them from 2.5.3 to 3.x. I'm still trying to understand a problem that port introduced regarding reconnecting, which means I need to understand pymodbus better.

@janiversen
Copy link
Collaborator

Let me know if I can help with your understanding….open a discussion if there are things you do not understand.

I am not a big expert, I have only been working directly on pymodbus for about a year, but I think I have been in nearly all parts of the code.

@janiversen
Copy link
Collaborator

also let me know if I can help persuade your employer to use more time on pymodbus. I have seen that a company depends on a project if often want to have an employe connected to the project.

You have earned committer status, if you want it, being the fifth most active on the project: https://github.com/riptideio/pymodbus/graphs/contributors
Your PR might be small, but they are good and I use very little review time.

Please let me know what you think.

alexrudd2 added a commit to alexrudd2/pymodbus that referenced this pull request Feb 21, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants