Tidy Errors Integration #26

jerneyio · 2015-09-27T00:39:50Z

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: servo/servo#7480

highfive · 2015-09-27T00:39:52Z

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 · 2015-09-27T00:39:53Z

Warning

These commits modify unsafe code. Please review it carefully!

jdm · 2015-09-28T22:09:33Z

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 · 2015-09-28T22:17:30Z

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

jerneyio · 2015-09-28T23:05:53Z

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

jdm · 2015-10-01T04:38:26Z

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 · 2015-10-01T10:59:45Z

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 · 2015-10-01T11:16:58Z

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 · 2015-10-01T19:28:57Z

@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 · 2015-10-05T20:07:38Z

errorlogparser.py

+        error_re = "\\x1b\[94m(.+?)\\x1b\[0m:\\x1b\[93m(.+?)\\x1b\[0m:\s\\x1b\[91m(.+?)(?:\\x1b\[0m|$)"
+        cont_comment_re = "\t\\x1b\[\d{2}m(.+?)\\x1b\[0m"
+        # error_re = "File:\s(.+?)\sLine:\s(.+?)\sComment:\s(.+)"
+        # cont_comment_re = "(\t.+)"


What is the purpose of these commented lines?

Ah, left over from personal testing. I will remove them.

jdm · 2015-10-05T20:58:44Z

payloadhandler.py

+        else:
+            pass
+
+    def manage_pr_state(self, issue, payload):


_manage_pr_state?

jdm · 2015-10-05T22:16:52Z

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 · 2015-10-06T22:08:50Z

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 · 2015-10-18T04:35:13Z

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 · 2015-11-26T22:00:01Z

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

frewsxcv · 2015-12-13T20:10:38Z

errorlogparser.py

+        err_match = None
+        i = 0
+
+        while not err_match and i < len(log_list):


This might be better structured as a standard for log in log_list: with a if err_match: conditional inside

closes servo#26

jerneyio added 4 commits September 16, 2015 08:23

integrating Tidy errors

de2fa5a

refactoring completed. need to make sure tests pass

008b3e6

further refactoring

43fdbda

Some further refactoring. Tests completed

a31245d

highfive added the S-awaiting-review label Sep 27, 2015

jerneyio added 3 commits September 26, 2015 20:43

fixup! Some further refactoring. Tests completed

1e47cc5

Do not re-post existing comments

4ac8544

Removed tests for internal code

8b7c65d

jerneyio mentioned this pull request Sep 27, 2015

Add a sample config file #21

Closed

jerneyio closed this Sep 28, 2015

jerneyio reopened this Sep 28, 2015

jdm reviewed Oct 5, 2015
View reviewed changes

payloadhandler.py

else:

pass

def manage_pr_state(self, issue, payload):

Copy link

Member

jdm Oct 5, 2015

Choose a reason for hiding this comment

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

_manage_pr_state?

jdm mentioned this pull request Nov 17, 2015

Highlight some easy style nits #2

Closed

frewsxcv reviewed Dec 13, 2015
View reviewed changes

jdm mentioned this pull request Apr 4, 2016

Add the S-needs-code-changes if travis build fails #28

Open

Mark-Simulacrum pushed a commit to Mark-Simulacrum/highfive that referenced this pull request Sep 8, 2020

Allow all collaborators to change reviewer

a1221c9

closes servo#26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tidy Errors Integration #26

Tidy Errors Integration #26

jerneyio commented Sep 27, 2015

highfive commented Sep 27, 2015

highfive commented Sep 27, 2015

jdm commented Sep 28, 2015

jerneyio commented Sep 28, 2015

jerneyio commented Sep 28, 2015

jdm commented Oct 1, 2015

jerneyio commented Oct 1, 2015

jerneyio commented Oct 1, 2015

jerneyio commented Oct 1, 2015

jdm Oct 5, 2015

jerneyio Oct 6, 2015

jdm Oct 5, 2015

jdm commented Oct 5, 2015

jerneyio commented Oct 6, 2015

jdm commented Oct 18, 2015

jdm commented Nov 26, 2015

frewsxcv Dec 13, 2015

Tidy Errors Integration #26

Are you sure you want to change the base?

Tidy Errors Integration #26

Conversation

jerneyio commented Sep 27, 2015

highfive commented Sep 27, 2015

highfive commented Sep 27, 2015

jdm commented Sep 28, 2015

jerneyio commented Sep 28, 2015

jerneyio commented Sep 28, 2015

jdm commented Oct 1, 2015

jerneyio commented Oct 1, 2015

jerneyio commented Oct 1, 2015

jerneyio commented Oct 1, 2015

jdm Oct 5, 2015

Choose a reason for hiding this comment

jerneyio Oct 6, 2015

Choose a reason for hiding this comment

jdm Oct 5, 2015

Choose a reason for hiding this comment

jdm commented Oct 5, 2015

jerneyio commented Oct 6, 2015

jdm commented Oct 18, 2015

jdm commented Nov 26, 2015

frewsxcv Dec 13, 2015

Choose a reason for hiding this comment