Tidy Errors Integration

[jerneyio]

A lot of refactoring took place, mostly moving code into classes and removing existing tests in favor of using python's unittest module.

I still need to determine a way to test this to make sure the CGI Script reacts properly to the json inputs (possibly just integration tests).

PR for: https://github.com/servo/servo/issues/7480

[highfive]

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @test_user_selection_ignore_this (or someone else) soon.

[highfive]

Warning

• These commits modify unsafe code. Please review it carefully!

[jdm]

Thanks for doing this work! I'm not sure precisely when I'll have time to review it, but my intention is to do so this week!

[jerneyio]

Sounds good to me. I'll give you a shout next week if I haven't heard back from you by then.

[jerneyio]

Well now I know what the 'Close and comment' button does...

[jdm]

Wow. I just glanced through each of the commits, and this looks like a fairly complicated series of refactorings and test harness changes. Any chance you could summarize the new design so I can orient my feedback appropriately? Additionally, where can I learn more about the mocking and @patch stuff? I have ~0 experience with idiomatic testing in Python.

[jerneyio]

Not a problem. I'll address Mock and patch first.

You can learn more about Python's mock library here and here. The reason behind using the library is twofold:

• Mocks provide a means to test functions with side-effects (like all the REST API calls) without actually performing the side-effects. Mock does this by recording calls to mocked functions so we can make assertions about how the function was called. With mocks, we can test methods that use REST API calls without a dependency on network connection and the API running.
• Patch allows us set the return value (among other things) for specific modules and their methods. We need this for methods that use random, for example, so we can fix the output of calls to random.choice for testing.

[jerneyio]

Here's an overview of the design:

• payloadreceiver.py - receives JSON payloads. Entry point to the application.
• payloadhandler.py -
  • TravisPayloadHandler - this class handles JSON payloads received from TravisCI
  • GithubPayloadHandler - this class handles JSON payloads received from Github
• githubapiprovider.py -
  • GithubApiProvider - this class handles requests to the Github API. This is largely unchanged from the existing code except for the addition of methods that get and post review comments.
• travisciapiprovider.py -
  • TravisCiApiProvider - this class handles requests to the Travis CI API.
• errorparser.py -
  • ServoErrorParser - this class handles parsing the log file generated by Travis CI's calls to tidy.py, and return the errors in a structured format.
• test.py - handles all the testing
• resources/* - these files are used for testing. I left the JSON files from the previous tests in there in case we need to do integration testing.

If you'd like anymore info, or have any questions, just let me know.

[jerneyio]

@jdm, if you'd rather chat about the design, tests, etc., on the Servo IRC, just let me know. I'm free most days after 5:00 PM EST

[jdm]

Ok, I finally finished reading the changes; apologies for the long delay! Thank you again for tackling this, and for spending time to improve the architecture of the code! Generally, I think many of the changes are for the better - we've got some decent separation of concerns now, and it looks like it's easier to both read and reason about in many cases. I'm less clear on the benefits of the test changes; I hope the comments I left in test.py are able to explain my concerns effectively. I am particularly interested in restoring the integration tests that previously existed; I believe those tests verify many of the properties of the bot's behaviour that are most important, and they do so without focusing on the actual implementation. Does that make sense?

[jerneyio]

No worries on the delay, I appreciate you taking the time to look over the code. Thank you for the valuable feedback, as well. Outside of test.py, I believe we're on the same page with the changes that should be made.

As far as test.py is concerned, I'd like to discuss what kind of tests should be run and where. I'm relatively new to testing, and after reviewing your comments I see I may have gone on a unit testing rampage in an effort to get as much code coverage as possible. Before I do any refactoring of test.py, I'd like to make sure we agree on what should be done. Here's what I propose:

• Tests that assert NotImpelementedError is raised should be removed. They don't buy us much.
• Tests on PayloadHandler classes should be removed in favor of integration tests that cover different use cases. This would include bringing back the previously existing integrations tests for GithubPayloadHandler and adding integration tests for TravisPayloadHandler
• Tests for API Providers do seem to be too tightly coupled to implementation details. Should we test them at all? All tests would essentially be asserting that the correct API call was made given a set of input.

What do you think?

[jdm]

Guh, sorry about the long delay.

• I agree
• Makes sense to me
• My experience has been that the API-related code in highfive gets written once and doesn't change, and when I'm writing it I don't necessarily have a model of correct behaviour that I could use to write tests. This suggests to me that it's not worth testing at that level, like you said.

[jdm]

I plan to rebase this on top of #33 once those changes merge, for the record!