Skip to content

Move to Github Actions and python version upgrade #238

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

Closed
wants to merge 14 commits into from

Conversation

shiplu
Copy link
Contributor

@shiplu shiplu commented May 5, 2021

I wanted to fix the Travis build errors and support Python 3.7. That lead me to many other changes. Of course, you can frown.
I used Google BigQuery to check the PyPI download statistics of zerorpc before dropping support of old python versions.

  • Dropped 2.6, 3.4, 3.5 support
  • Introduced 3.6, 3.7, 3.8 and 3.9
  • Use "bionic" on for the travis build
  • Pass 'async' as ** dict item in the test so it's compatible with newer
    versions of python
  • Use pyzmq 16.0.4+ as the old versions cannot be built in python 3.7
  • Support modern repr of BaseException
    BaseException doesn't add trailing comma from version 3.7.0.
    Hence the test case without ',' is added as well.
    See also: https://bugs.python.org/issue30399

@shiplu
Copy link
Contributor Author

shiplu commented May 5, 2021

Not sure why the Travis checks are not triggered. It triggered well in my fork

@shiplu shiplu changed the title Upgrade python version Fix travis build using python version upgrade May 20, 2021
@gabrielgrant
Copy link

Thanks for this @shiplu

+1 on dropping 2.6 support, it's been EOLed for years now. Rest of the changes look good to me, but haven't been through this with a fine-toothed comb.

@bombela any idea why Travis isn't getting triggered? Is that something you can do manually?

@shiplu
Copy link
Contributor Author

shiplu commented Jun 28, 2021

Maybe travis is dropping support. @bombela A small nudge. Would you have time to review this?

@shiplu shiplu force-pushed the upgrade-python-version branch 2 times, most recently from c42d5bf to fb1e7f9 Compare July 8, 2021 21:42
@shiplu shiplu changed the title Fix travis build using python version upgrade Move to Github Actions and python version upgrade Jul 8, 2021
@shiplu
Copy link
Contributor Author

shiplu commented Jul 8, 2021

I have removed travis support and add github actions instead. The tests are running in my own PR. Still now sure why it's not being triggered in this PR.

@@ -162,7 +161,7 @@ def test_heartbeat_can_open_channel_client_close():
print('SERVER LOST CLIENT :)')
server_hbchan.close()
server.close()

client.close()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test test_heartbeat_can_open_channel_client_close fails in Python 3.9 if the client is closed before the server heartbeat channel is closed. Looking at the other tests, it seemed to me that client.close() should be called after server.close().

Copy link
Member

Choose a reason for hiding this comment

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

This defeats the purpose of this test. The point is to have the client go away. And the server to detect this by the loss of heartbeats.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see. Then I'll change it back. What I found that the test fails on python 3.9 if the client is closed before. Any idea why it fails on Python 3.9?

Copy link
Member

Choose a reason for hiding this comment

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

The test itself is successful. You can clearly see the message "CLOSE CLIENT SOCKET!!!". Followed by "SERVER LOST CLIENT :)". This is the expected behavior.

Somehow the test is marked as failed. Maybe the assert_raises is not doing its job.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the confirmation. I'll see if I can fix it!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems the issue is on nosetests. I run many times these tests on python 3.9. It never fails. Only on github it fails. Then I found one of the tests `` passing with ok status.

tests.test_buffered_channel.test_congestion_control_server_pushing ... Traceback (most recent call last):
  File "src/gevent/greenlet.py", line 906, in gevent._gevent_cgree nlet.Greenlet.run
  File "/__w/zerorpc-python/zerorpc-python/tests/test_buffered_channel.py", line 429, in server_do
    server_bufchan.emit('coucou', x, timeout=0)  # will fail when x == 1
  File "/usr/local/lib/python3.9/unittest/case.py", line 226, in __exit__
    self._raiseFailure("{} not raised".format(exc_name))
  File "/usr/local/lib/python3.9/unittest/case.py", line 163, in _raiseFailure
    raise self.test_case.failureException(msg)
AssertionError: TimeoutExpired not raised
2021-07-13T21:28:53Z <Greenlet at 0x7efdea2d5260: server_do> failed with AssertionError

ok

AssertionError and ok status together shouldn't be right.

The errors can be seen in my github action build log

So, I think for now we drop supporting python 3.9 (removing the entry from ci.yml and not adding in setup.py). Then probably we can migrate from nosetests to pytest which will enable us to support 3.9 in near future!

@shiplu shiplu force-pushed the upgrade-python-version branch 3 times, most recently from 684c64c to e7aec19 Compare July 11, 2021 00:39
shiplu added 2 commits July 13, 2021 23:20
- Dropped 2.6, 3.4, 3.5 support
- Introduced 3.6, 3.7, 3.8
- Use "bionic" on for the travis build
- Pass 'async' as ** dict item in the test so it's compatible with newer
versions of python
- Use pyzmq 16.0.4+ as the old versions cannot be built in python 3.7
- Support modern repr of BaseException
  BaseException doesn't add trailing comma from version 3.7.0.
  Hence the test case without ',' is added as well.
  See also: https://bugs.python.org/issue30399
@shiplu shiplu force-pushed the upgrade-python-version branch from 0359415 to 3acf67a Compare July 13, 2021 21:20
@shiplu shiplu force-pushed the upgrade-python-version branch from 8878655 to 304ed37 Compare July 13, 2021 22:32
@shiplu
Copy link
Contributor Author

shiplu commented Aug 31, 2021

@bombela Any comments?

@chombourger
Copy link
Contributor

Not sure what @bombela thinks but it would IMO help if we break this PR into smaller ones (e.g. raise a separate PR to fix Flake8 errors)

@bombela
Copy link
Member

bombela commented Jan 5, 2022

I agree, one PR per topic. In order of reverse dependency of course.

Dropping python 2.6 might be reasonable. For other versions of python 3, I am not so sure. I hate it when things suddenly stop working after an upgrade because people ditch backward compatibility.

@shiplu
Copy link
Contributor Author

shiplu commented Aug 22, 2022

Closing this PR. I woudln't have time to work on it. Feel free to take ownership.

@shiplu shiplu closed this Aug 22, 2022
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.

4 participants