search

unicef / kindly

GNU Affero General Public License v3.0
24 stars 17 forks source link

CI GitHub Actions for unit tests #91

Closed sabinevidal closed 2 years ago

sabinevidal commented 2 years ago

Addresses issue #80 GitHub Action for running Python Unit tests for API (and testing build for Python 3.8)

sabinevidal commented 2 years ago

@lacabra Ok, all working now! python get_model.py was the trick.

Have named file ci.yml to differentiate from the main.yml file dedicated to the docker action. Possibility to add GitHub action for pylint created in #90

lacabra commented 2 years ago

Ready for review? Let's merge this one first, and then we can add the pylint one. I'm surprised that it doesn't run the file, but I suppose it's because it isn't merged yet

sabinevidal commented 2 years ago

@lacabra Can remove the duplication, was actually looking into actions/upload-artifact to extend the build to the test. Have some ideas for extending this, ie including testing for Python 3.6 build as we have separate requirements for it. I picked up a couple of issues with dependencies just while trying to test the actual unit tests, so definitely worth having a separate stage for the build, just incase someone tries to upgrade/add a dependency, which may work on local, but may actually conflict. This was how I picked up the nltc typo #86

And separate so it's easier to see it failed in build, not just test

sabinevidal commented 2 years ago

@lacabra

Oh, and the duplication was originally to be able to test multiple python versions in build, and then focus on 3.8 in the test as the other versions weren't necessarily relevant and would just slow it down

lacabra commented 2 years ago

@sabinevidal Ah, I see; makes sense. Well, let's find a balance between being comprehensive in testing and keeping it simple.

One option is to rename build to build-3.6 and just test whether it builds under 3.6 with requirements.python-3.6.8.txt and leave test as is which tests both 3.8 and the actual unit tests (while removing the dependency to the previous build stage). I am not concerned about splitting it into different stages for the sake of seeing where it brakes, as when it brakes it is simple enough to look at the failed build and debug.

sabinevidal commented 2 years ago

@lacabra Ok cool, will quickly make those changes and test!

sabinevidal commented 2 years ago

@lacabra Changes made and tests pass ✅

lacabra commented 2 years ago

FYI, as far as I can see the tests pass in this PR because they are being skipped (if you click on the green checkmark icon you will see) because the ones you are adding don't run yet. But after merging, I see they pass on main, so we are all set! 💪

Thanks again!

sabinevidal commented 2 years ago

Great! Tested a second time on my bug testing branch and they run there so feeling confident 😅💪 This was a great learning exercise though! Appreciated the chance to both learn about Github Actions and also dig into debugging in the transformers files to figure out the ./model path issue.