Add python tests

[mariekers]

Make sure python code works and isn't broken by changes

Dependent on #59

Not an expert but here's some tips on writing python tests

[themightychris]

I'd also consider this one done already given that you broke out #61 too -- that covers all that's left

(feel free to reopen with more detail if I'm misreading that)

[mariekers]

Adding unit tests to the functions within app.py

workflow in git actions does a build check - can we do tests on individual functions?

[themightychris]

A python workflow was already implemented into GitHub actions in #52:

• pytest gets run at the end of the pythonapp workflow: https://github.com/CodeForPhilly/chime/pull/52/files#diff-7e3bc64c9dd0eb02c0cac6101ee4e286R40
• test_app.py was created with a single sample assertion: https://github.com/CodeForPhilly/chime/pull/52/files#diff-dda76c240c44e9a17f6534f1a4065235R4

The TODO at this point is just to actually add meaningful tests to test_app.py. What are some examples of function this issue might cover testing that would not be part of #61?

I just updated the testing section of the docs: https://codeforphilly.github.io/chime/contributing/app-dev.html#testing

[themightychris]

Over here you can see how even though the current test_app.py only has one trivial assertion, that alone covers a lot of ground in at least ensuring the application and its deps can be loaded: https://github.com/CodeForPhilly/chime/pull/55/checks?check_run_id=515843837

[garysieling]

I thought about this a bit, here what I would propose - this should be a pretty straightforward task for someone who knows python reasonably well.

1. Make a test that takes runs using inputs that you can pick in the UI - assert that the model produces the same results currently. Do something like 50 randomized selections of the current inputs. This would prevent regressions.

2. Test each input to the model for edge cases (what happens if I put in 0, a really large number, a fraction, if there are any of those)

3. Track down the original paper for this model and write a test that replicate their results

[garysieling]

Possible that I've duplicated the intent of issue #61 in the above

[pjhoberman]

I'm going to attempt some tests here, and see if the charting functions return values so that those can be tested (since I have no idea how to make Python look at an actual visual and see if it worked...).

Question: Would you rather this be part of the github actions, or part of the developer flow (git hooks) so that devs can't even push a change without running and passing a test?

[themightychris]

@pjhoberman we already have pytest gating pull requests, so my sense is we should just start with adding tests to test_app.py

I don't think we need to mess with running it via githooks right now, devs being able to run pytest and then GitHub running that against PRs should be a great start

[garysieling]

@pjhoberman since there's a range of skill level of volunteers, I think it's fine to gate at the last possible point (run tests on github before PR merges) - I don't want anyone to give up if they get tripped up on tests

[garysieling]

Also note based on the slack traffic there's a concurrent refactoring of app.py which would make this ticket a lot easier

[sam-writer]

@garysieling I think you are right, and unit testing will be easier after merging #55.

I like this suggestion. I think verifying the models and data aren't broken is most important. However, even after the refactor, there is some coupling of logic and presentation (the framework we are using, Streamlit, makes it pretty hard to fully decouple the two). So, if we want to encode the manual testing procedure, I think that sounds like a puppeteer/selenium thing to me. Or even kasaya which seems like it might be a way to get QA help from non-coders.

[garysieling]

Made a new ticket for Selenium type testing, I think that is a good idea as well

[pjhoberman]

I’ll get on the unit tests today (I’m on baby duty so might be mid day before I push anything). I’ll try to break them out so some tests get pushed earlier. If there’s anything specific anyone wants covered earlier let me know. I can also help with selenium but I’ll leave that open for now so others can join in on the fun!

…

On Thu, Mar 19, 2020 at 6:18 AM Gary Sieling ***@***.***> wrote: Made a new ticket for Selenium type testing, I think that is a good idea as well — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#60 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAJIJBA7NS6QZ5GI2PCWRLRIIERBANCNFSM4LOV6EOQ> .

[pjhoberman]

• add to test_sir including edge cases. Ensure things that shouldn't work don't work (like negative numbers in some spots) -- Add to test_sir #132
• add more tests around sir function - need guidance from others on what should break it -- Added assertions to test_sir #151
• add to test_sim_sirincluding edge cases. Ensure things that shouldn't work don't work (like negative numbers in some spots) -- Added assertions to test_sir #151
• add to test_new_admissions_chart - find a way to break the chart and make sure the test captures that
• create test_admitted_patients_chart and follow same patterns as test_new_admissions_chart

[quinn-dougherty]

When can we close this? it's a really broad issue

[garysieling]

I think it makes sense to resolve it and make smaller tickets for whatever is left- I can take a look at doing that tomorrow

[pjhoberman]

That last push was all I needed out of this ticket. I think further tickets are a great idea.

…

On Sat, Mar 21, 2020 at 8:51 PM Gary Sieling ***@***.***> wrote: I think it makes sense to resolve it and make smaller tickets for whatever is left- I can take a look at doing that tomorrow — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#60 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAJIJHM56E7URYQFCBKWIDRIV4LTANCNFSM4LOV6EOQ> .

[garysieling]

Great - I'll close this now and make tickets if there was anything I was hoping for that isn't covered