#11616 disttrial worker test improvements #11617
Conversation
These are handy for writing hamcrest-based tests for a lot of trial code.
The previous implementation used unnecessary fakes and encoded unnecessary implementation details as well as unnecessarily duplicating in-memory ITransport implementation available in other testing libraries in Twisted. This is an almost-no-behavior-change commit that just makes the tests simpler and exercises the code under test in a more realistic way.
| ), | ||
| ), | ||
| ), | ||
| ), |
There was a problem hiding this comment.
I am not a big fan of these generic assertions.
we can leave this as it is, since matched_result is already in trunk.
I find the name a bit too generic.
Something to my taste.
assert_than(result, is_testrun_failure(case, type=AssertionError, value=WorkerException("pyunit failure")
and all the extra isTuple, equal_to and the general structure is handled by the dedicated matcher.
From my experience, with these specialiez markers it was easier to refactor the internal structure of a code as you might only need to update the specialized marker, without updating each test.
There was a problem hiding this comment.
This overlaps a bit with the comment above and I left a reply there.
| frames=["file.py", "invalid code", "3"], | ||
| case = pyunitcases.PyUnitTest("test_error") | ||
| result = self.workerRunTest(case) | ||
| assert_that( |
There was a problem hiding this comment.
at least for me, this embedded structure is not easier to read ...
but maybe is just my bias for old-school xunit type assertions.
- self.assertEqual(failure.type, ValueError)
- self.assertEqual(failure.value, WorkerException("error"))
+ self.assertTestRunFailure(self.result, ValueError, WorkerException("error"))For this type of assertion, you only focus on the end/high-level result, and ignore the internal structure of a test case.
It might be an advantage or disadvantage.
I usually write a test in which the internal structure is validated, and then all the other tests only check the high-level values, ignore the internal structure.
With the generic matches_result you need to remember the full structure of the test case result, each time you write the test.
There was a problem hiding this comment.
at least for me, this embedded structure is not easier to read ...
I don't object to that. I think a lot of it is about familiarity and experience but it is also true that matcher-style tends to have fewer larger expressions rather than more smaller expressions.
Larger expressions in this style probably often take more practice to become comfortable reading. The benefit that you get is when multiple parts of the value are wrong, you get to learn about all of them after one test run instead of only the one related to the very first assertion.
testtools allows the best of both worlds with an expectThat function which can add a test failure and let the test keep running. But Twisted decided to go with pyhamcrest instead...
Anyhow, I'm open to improving the readability of these tests before merging.
There was a problem hiding this comment.
- self.assertTestRunFailure(self.result, ValueError, WorkerException("error"))
Looking at this idea specifically in more detail, I don't see a good way to make it work. The trouble is that assertTestRunFailure isn't reusable. test_runError, test_runFailure, test_runSkip, etc all look similar but they differ in slight ways.
If we rename the suggested function to assertTestRunError (since this case is about test suite "errors" rather than "failures") and we implemented it however (doesn't matter if we use hamcrest or not) then it is usable by exactly one test, test_runError. So we need another one for test_runFailure, and another for test_runSkip, etc.
In this case I don't think there's any benefit to abstracting the details away from the test. The purpose of the test method is exactly to encapsulate these details.
I can flatten the hamcrest code so it does repeated calls to assert_that instead of a single call with a complex matcher. I think that change makes it easier for folks without a lot of practice writing and reading these and it doesn't hurt runtime properties that much probably.
I don't think it makes sense to take the details of those assertions out of the test methods though. Or at least, I don't see a way that doesn't just lead to more work at development time and more work for future readers of the code.
There was a problem hiding this comment.
I flattened the matcher in 6deb9f1 - I'm curious if you find that to be any better
There was a problem hiding this comment.
thanks for the info and the updated.
Agree.
The flattened version looks easier to read, at a first read.
I am sure that after reading and looking at the code a few more times, the nested structures will look much easier to read.
I just wanted to leave a "raw" feedback.
I guess that other people might want to help contribute the Twisted, but then they found some code that they don't understand and then they give up.
I am searching for the formula for creating an open source project that is easy to contribute to, while still keeping high code quality standards.
There was a problem hiding this comment.
I now see that this PR was not ready for review.
On a general note, I am -1 for using hypothesis in twisted, especially for general code.
It adds another thing that you must learn and be familiar with, before contributing to the project.
And for people not very familiar with the library, might be a reason to write invalid tests.
exarkun
changed the title
make sure stopTest is called only once and with the right argument
|
needs-review |
There was a problem hiding this comment.
I think that this can be merged.
I am not a big fan of hamcrest , but I am not -1 on it.
Jean-Paul, if you think that testtool.assertThat provides a better dev experience in terms of writing matches and diffs, I think we can still consider a switch.
Using simple hypothesis strategies can make the code easier to use and encourage developers to write tests with explicit types.
I saw many bugs (in python 2.7) that were not discovered by a test, because the test was using something like received = 'binary-data' ... but that binary-data was a valid text that survived an accidental UTF-8 decode on it.
And in prod you have real binary data and then UTF-8 decoding fails.
I only did a quick review.
I don't know if anyone will have time to review it.
I think is best to have it merge, than keep this PR in the review queue for many weeks
| frames=["file.py", "invalid code", "3"], | ||
| case = pyunitcases.PyUnitTest("test_error") | ||
| result = self.workerRunTest(case) | ||
| assert_that( |
There was a problem hiding this comment.
thanks for the info and the updated.
Agree.
The flattened version looks easier to read, at a first read.
I am sure that after reading and looking at the code a few more times, the nested structures will look much easier to read.
I just wanted to leave a "raw" feedback.
I guess that other people might want to help contribute the Twisted, but then they found some code that they don't understand and then they give up.
I am searching for the formula for creating an open source project that is easy to contribute to, while still keeping high code quality standards.
| @@ -185,7 +185,7 @@ def addExpectedFailure( | |||
| """ | |||
| Add an expected failure to the reporter. | |||
| """ | |||
| _todo = Todo(todo) | |||
| _todo = Todo("<unknown>" if todo is None else todo) | |||
There was a problem hiding this comment.
I guess that this change was done to make it easier to remove Todo.__init__ while defining reason: str and not Optional[str]
I am happy to always have a string as the reason.
There was a problem hiding this comment.
Yep. I also took a hint from the existing docstring for Todo which claimed reason was a string and said nothing about None.
This also removes the need for isTuple
They disagree with the inherited type which makes TestResult a very poorly behaved subclass. But, so it is.
Thanks for the reviews @adiroiban . I went ahead and flattened the hamcrest usage in the other I think having a richer matching library can also help us out and right now we're just starting to build one up for trial so we have some more work to do in that department. Also, I think "it's hard to describe what result I want" is a bit of a code smell for "what you want is not great" so as we find it is hard to write good matchers we might decide we want to introduce better data structures into the implementation to make it easier. |
|
It looks like the conch manhole tests are somewhat reliably broken on Windows now. :/ |
for more information, see https://pre-commit.ci
the broken test doesn't try to write to sys.stdout so it doesn't matter (I hope!!)
…to 11616-test_worker-improvements
for more information, see https://pre-commit.ci
…to 11616-test_worker-improvements
the failure went away on CI. will it still be gone in the next run? who can say
Fixes #11616