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

Bugfix/tests #72

Merged
merged 9 commits into from
May 2, 2021
Merged

Bugfix/tests #72

merged 9 commits into from
May 2, 2021

Conversation

Kick1911
Copy link
Contributor

Fixing tests to run on python 3.7

  • All tests pass on tox (besides the ones that are skipped)
  • 1 test fails on pytest --use-docker tests/ (tests/apps/test_measure.py::test_with_orig[CSVStore])

Main changes

  • Tag for docker image safarov/freeswitch must now be specified. Using 1.10.3

@goodboy
Copy link
Member

goodboy commented Nov 16, 2020

@Kick1911 oh wow thanks for doing this 🥳

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

Talk soon 🏄

@goodboy
Copy link
Member

goodboy commented Nov 16, 2020

@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.

@@ -33,7 +33,7 @@ def con(fshost, loop):
@pytest.mark.parametrize(
'password, expect_auth',
[('doggy', False), ('ClueCon', True)],
ids=lambda item: "pw={}, expect_auth={}".format(*item),
# ids=lambda item: "pw={}, expect_auth={}".format(*item),
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason this was removed?

It's not a big deal but iirc these ids tags sure are handy when doing manual runs of the test suite from console.

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 have added it back in, but it works differently now.

Copy link
Member

@goodboy goodboy left a comment

Choose a reason for hiding this comment

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

@sudonum yeah this looks great to me 👍

The only thing I'd love to see in this PR is tweaks to get CI working again.
Let me see if I can figure that out this afternoon.

@goodboy
Copy link
Member

goodboy commented Nov 16, 2020

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
Copy link
Member

goodboy commented Nov 16, 2020

There, travis needed to be re-synced 🙄

@Kick1911
Copy link
Contributor Author

Kick1911 commented Nov 16, 2020

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

@goodboy
Copy link
Member

goodboy commented Nov 17, 2020

@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
Copy link
Member

goodboy commented Nov 17, 2020

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
Copy link
Member

goodboy commented Apr 8, 2021

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

This project needs some life brought back 😂

@Kick1911
Copy link
Contributor Author

Kick1911 commented Apr 8, 2021

@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:
d6b56c1

@goodboy
Copy link
Member

goodboy commented Apr 9, 2021

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
Copy link
Contributor Author

Kick1911 commented Apr 24, 2021

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
Copy link
Contributor Author

@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
Copy link
Member

goodboy commented Apr 30, 2021

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!

@@ -67,7 +67,7 @@ def containers(request, confdir):
if request.config.option.usedocker:
docker = request.getfixturevalue('dockerctl')
with docker.run(
'safarov/freeswitch',
'safarov/freeswitch:1.10.3',
Copy link
Member

Choose a reason for hiding this comment

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

We should probably have a CD process eventually that keeps us in sync with core fs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like the maintainer added a latest tag 4 months ago. I'll use that tag instead.

@goodboy
Copy link
Member

goodboy commented Apr 30, 2021

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
Copy link
Contributor Author

Kick1911 commented May 1, 2021

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

@@ -67,7 +67,7 @@ def containers(request, confdir):
if request.config.option.usedocker:
docker = request.getfixturevalue('dockerctl')
with docker.run(
'safarov/freeswitch:1.10.3',
'safarov/freeswitch:latest',
Copy link
Member

Choose a reason for hiding this comment

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

🥳

@@ -271,6 +271,11 @@ def test_with_orig(get_orig, measure, storer):

# configure max calls originated to length of of storer buffer
cdr_storer = orig.measurers['CDR'].storer

if (cdr_storer.storetype.__name__ == 'CSVStore'
Copy link
Member

Choose a reason for hiding this comment

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

Not sure what's up here but we're skipping this measurement test for now since it's flaky in github actions.

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 was to skip that storertype, but not skip the other storertype. I didn't know if I should skip the test completely for all python 3.7+ or not.

@goodboy goodboy mentioned this pull request May 2, 2021
@goodboy
Copy link
Member

goodboy commented May 2, 2021

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
Copy link
Contributor Author

Kick1911 commented May 2, 2021

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
Copy link
Contributor Author

Kick1911 commented May 2, 2021

@goodboy Done

@goodboy goodboy merged commit 0e7aa9f into friends-of-freeswitch:master May 2, 2021
@goodboy
Copy link
Member

goodboy commented May 2, 2021

@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
Copy link
Member

goodboy commented May 2, 2021

@goodboy goodboy mentioned this pull request May 4, 2021
5 tasks
@goodboy goodboy mentioned this pull request May 31, 2021
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.

3 participants