webcompat / webcompat-metrics-server

Server in charge of delivering different data to the webcompat-metrics-client
Mozilla Public License 2.0
6 stars 5 forks source link

Fixes #49 - Add unit tests for github webhook handler and helpers #56

Closed laghee closed 6 years ago

laghee commented 6 years ago

Still working on this. There's lots of documentation on how to mock api/request data in python and much less on database operations. A lot of this may need to be figured out somewhat trial and error. 🤔

Any suggestions or resource links for mocking/patching database operations more than welcome! (Any comments or suggestions on code-in-progress also appreciated.)

laghee commented 6 years ago

@karlcow OK! They might be ugly in places and can certainly be improved, but all 29 of these suckers now pass locally. (The test file is really long. Please don't kill me... 😜 Could also be useful as a soporific if you ever suffer from insomnia!)

miketaylr commented 6 years ago

@laghee do you know what's up with the following failure?

...........E
======================================================================
ERROR: tests.unit.test_webhooks (nose2.loader.ModuleImportFailure)
----------------------------------------------------------------------
ImportError: Failed to import test module: tests.unit.test_webhooks
Traceback (most recent call last):
  File "/home/circleci/repo/venv/lib/python3.6/site-packages/nose2/plugins/loader/discovery.py", line 201, in _find_tests_in_file
    module = util.module_from_name(module_name)
  File "/home/circleci/repo/venv/lib/python3.6/site-packages/nose2/util.py", line 77, in module_from_name
    __import__(name)
  File "/home/circleci/repo/tests/unit/test_webhooks.py", line 21, in <module>
    from ochazuke.webhook import helpers
ModuleNotFoundError: No module named 'ochazuke.webhook'
laghee commented 6 years ago

@miketaylr Hmm. I think it's because the module being tested is on the other branch. All these PRs are separate because I was trying to make it simpler. But they depend on each other, so I'm not sure how to make that work. I think I might need some git advice...

karlcow commented 6 years ago

@laghee @miketaylr

But they depend on each other, so I'm not sure how to make that work. I think I might need some git advice...

I don't think git will help that much at this stage. It's not the modular way I was thinking about when I was thinking about smaller PRs. 😃 but that's my bad for making it not clearer.

Right now the code is split in big interdependent PRs organized by logical code organization. What I was hinting at was organizing PRs by little functional issues. Let's say for example handling the HTTP response and its tests. Then going to the next stage in processing data and its tests. etc. :)

All of that said we will survive. Be a big unique PR or merging them one by one it will take time. We can't really review the tests currently because they are dependent on the rest of the code. So we have to go through others first. I'm planning to work on this a bit today.

karlcow commented 6 years ago

@laghee could you resolved the conflicting files by rebasing on the latest master.

laghee commented 6 years ago

Right now the code is split in big interdependent PRs organized by logical code organization. What I was hinting at was organizing PRs by little functional issues. Let's say for example handling the HTTP response and its tests. Then going to the next stage in processing data and its tests. etc. :)

Ahhh. Ooops. I get it now.

All of that said we will survive. Be a big unique PR or merging them one by one it will take time. We can't really review the tests currently because they are dependent on the rest of the code. So we have to go through others first.

I'll make the artisanalmustard.example.com change and rebase for now.

laghee commented 6 years ago

@karlcow OK, changes made. I also took out the db statements since they're not needed with mocking and they were freaking out CircleCI. (Looks like the setup I added for accessing the postgres db in Circle isn't quite right (issue #57), which I'll need to figure out when we start working on integration testing).

karlcow commented 6 years ago

Thanks a lot @laghee ❤️