tasks: Test posted statuses in mock PR and image refresh #592
Conversation
martinpitt
force-pushed
the
test-statuses
branch
from
5297db7 to
a770739
Compare
There was a problem hiding this comment.
I think this is fine. Usual suggestions below.
I'm also not a fan of the tone of the second paragraph of the commit message (although this is something I feel like I've been guilty of in the past). It's not immediately clear to me if the commit in question is doing the thing that it's talking about or just "enabling" it (with the expectation that it will be done in a future commit).
| @@ -39,6 +40,10 @@ class Handler(MockHandler): | |||
| self.send_error(404, 'Mock Not Found: ' + self.path) | |||
|
|
|||
| def do_POST(self): | |||
| if log: | |||
| print("POST", self.path, self.rfile.read1().decode(), file=log) | |||
There was a problem hiding this comment.
...why not (also) GET?
There was a problem hiding this comment.
This is mostly meant to validate actions. GET requests don't change the state, they are already sufficiently defined in terms of which paths do_GET handles in the test, and they are also somewhat of an implementation detail. I can add them to the log of course, but I wouldn't have anything to assert on them. For human inspection it's nice though, I agree.
Added.
tasks/mock-github
Outdated
| @@ -39,6 +40,10 @@ class Handler(MockHandler): | |||
| self.send_error(404, 'Mock Not Found: ' + self.path) | |||
|
|
|||
| def do_POST(self): | |||
| if log: | |||
| @@ -19,6 +19,7 @@ from task.test_mock_server import MockHandler, MockServer | |||
|
|
|||
| repo = None | |||
| sha = None | |||
| log = None | |||
There was a problem hiding this comment.
I wonder if you'd like to add some more types...
There was a problem hiding this comment.
TBH not today when we have a crapton of stuff to catch up with. I'd like to reserve this to the next weeks -- I also want to rewrite run-local.sh as proper pytest.
Teach `mock-github` to write a log of POST requests, and add some initial assertions. They are not very precise yet (that'll get cleaned up in the Python rewrite soon), but at least ensure that we post the statuses to the expected SHA. This enables us to create an integration test for a cross-project test in a future commit.
martinpitt
force-pushed
the
test-statuses
branch
from
a770739 to
39ac4c3
Compare
|
@allisonkarlitskaya Reworded the commit message and fixed some of your suggestions. Thanks! |
Teach
mock-githubto write a log of POST requests, and add some initial assertions. They are not very precise yet (that'll get cleaned up in the Python rewrite soon), but at least ensure that we post the statuses to the expected SHA.This enables us to create an integration test for a cross-project test in a future commit.