friends-of-freeswitch / switchio

asyncio powered FreeSWITCH cluster control
http://switchio.rtfd.io
Mozilla Public License 2.0
191 stars 28 forks source link

Bugfix/tests #72

Closed Kick1911 closed 3 years ago

Kick1911 commented 3 years ago

Fixing tests to run on python 3.7

Main changes

goodboy commented 3 years ago

@Kick1911 oh wow thanks for doing this :partying_face:

I'm going to take an in depth look this afternoon EST.

Talk soon :surfer:

goodboy commented 3 years ago

@Kick1911 one thing that could happen right now is re-enabling the CI run in travis, though likely flipping to GH actions would be more desirable for a load heavy test set like this.

I think we disabled it as part of #65.

goodboy commented 3 years ago

Huh, so can't remember if I disabled the CI run or not but the results from this branch are here: https://travis-ci.org/github/friends-of-freeswitch/switchio

Looks like for nightly we need to install a newer C++ compiler. I'm tempted to just move everything to GH actions since it's been far superior in a lot of ways and it's more juice for parallel builds.

goodboy commented 3 years ago

There, travis needed to be re-synced :roll_eyes:

Kick1911 commented 3 years ago

I am busy trying to get GH action to work on my fork. I am getting somewhere.

goodboy commented 3 years ago

@Kick1911 just looking at your actions run and it looks like we might be still hitting the same issue of needing a health check directive in the dockerfile as required by the pytest-dockerctl plugin.

I think this may be why the CI stopped working initially as well? I don't recall requiring it initially and maybe it was changes to docker-py/docker that allowed this to work before?

Maybe we can just stick in a flag to ignore the health check in the plugin or see if this freeswitch image can be changed to include it?

goodboy commented 3 years ago

Huh, the health check appears to be in the base image so i'm not entirely sure what's wrong here. It also is in the official image. Maybe it's this attr path is incorrect?

goodboy commented 3 years ago

@Kick1911 if you're confident in this I'm fine to merge.

This project needs some life brought back 😂

Kick1911 commented 3 years ago

@Kick1911 if you're confident in this I'm fine to merge.

This project needs some life brought back

@goodboy I am not completely happy with it yet. In my experimentation branch I realised that some changes I made are not backwards compatible. I realised this when manually running the tests on my laptop with different versions on python 3.x. So, I would really like the testing to work so we could not make mistakes like that. I think I will give it another try, this time making a docker on my laptop that runs docker and runs the tests. (I was using VMs to run the tests) Logic is if I can recreate the error on my laptop, it will be much easier to debug.

I made it backwards compatible here: https://github.com/friends-of-freeswitch/switchio/commit/d6b56c191c26cac2e5138c213677f4bda5eb529e

goodboy commented 3 years ago

I realised that some changes I made are not backwards compatible. I realised this when manually running the tests on my laptop with different versions on python 3.x. So, I would really like the testing to work so we could not make mistakes like that. I think I will give it another try, this time making a docker on my laptop that runs docker and runs the tests.

@Kick1911 sounds good. Just let me know when you're happy.

I'm pretty sure running the tests locally should be pretty straight forward. As mentioned I'm pretty sure the latest issue in CI is just some silly health check thing.

Kick1911 commented 3 years ago

Ok. Found out what the problem was by pure luck. Added the fix for the testing problem and backwards compatibility for < python 3.7. You can check how the tests run on our fork PR number 2. https://github.com/sudonum/switchio/pull/2/checks

All tests pass for python 3.6. One test fails for python 3.7 and 3.8. They fail on tests/apps/test_measure.py::test_with_orig[CSVStore] FAILED with a TimeoutError. I don't know what that is about. Maybe you @goodboy can guide me on that.

Kick1911 commented 3 years ago

@goodboy I think you can merge it now and enable the github action that I have added if you are happy. Unless you want to see all tests passing, then I can change it to only run tests for python 3.6 to get the nice green tick.

goodboy commented 3 years ago

hey @Kick1911 thanks for this work and sorry about 👻 ing again - I'm not paying attention to the GH feed as much as I should.

You can check how the tests run on our fork PR number 2.

Amazing. nice to see the suite run on 3.8 as well. I'm noticing that 3.6 and 3.7 runs hung and were cancelled? Any idea what's going on there?

then I can change it to only run tests for python 3.6 to get the nice green tick.

Weird i only see the 3.8 run actually terminating with the one error.

They fail on tests/apps/test_measure.py::test_with_orig[CSVStore] FAILED with a TimeoutError.

Can we maybe just skip this test for now and make an issue about it?

Thanks again for all this hard work!

goodboy commented 3 years ago

Hmm also, aren't we going to need a version pin in setup.py for 3.6?

Gonna be nice to get a new release out if we can get this all up and running again 🏄🏼

Kick1911 commented 3 years ago

@goodboy Ok, I think it is all good now. All tests pass. https://github.com/sudonum/switchio/runs/2484324517?check_suite_focus=true

goodboy commented 3 years ago

Created #74 to track otherwise I think this is ready to land.

@Kick1911 only last thing might be to change readme to say we're 3.6+ now yah?

Kick1911 commented 3 years ago

Created #74 to track otherwise I think this is ready to land.

@Kick1911 only last thing might be to change readme to say we're 3.6+ now yah?

Sure. Will do.

Kick1911 commented 3 years ago

@goodboy Done

goodboy commented 3 years ago

@Kick1911 oh shoot we probably need to move the CI readme badge to GH actions as well.

Feel free to PR that in if you want but no presh.

goodboy commented 3 years ago

master run : https://github.com/friends-of-freeswitch/switchio/actions/runs/804780948