[React Native] Add tests to paper renderer for measure, measureLayout #15323
Conversation
There was a problem hiding this comment.
Not sure how I feel about some of the assertions in the new tests. Going to think on it a bit.
I realize I approved a PR (#15126) recently with similar tests. I guess I didn't read the tests closely enough to notice the thing that's nagging me/
| const args = UIManager.measure.mock.calls[0]; | ||
| expect(successCallback).not.toBeCalled(); | ||
| args[1]('success'); | ||
| expect(successCallback).toBeCalledWith('success'); |
There was a problem hiding this comment.
This seems a little janky too.
There was a problem hiding this comment.
Here, I agree with you. The coupling is tight as a side effect of an array being used for the arguments instead of a hash with a named key. Given the design of the code, I’m not sure what else to do except make a test helper function that wraps this detail and give it an intention-revealing name.
There was a problem hiding this comment.
The specific reason I wanted to test this is because one of the UIManager measure calls takes the arguments fail, success, but the host component takes the arguments success, fail and flips the order when passed to the UIManager. I wanted to validate that this stays the correct order because it is trivially easy to break (or think that the existing code is wrong).
I can't assert that the callback is the callback that is passed in because it is wrapped, so calling it and validating the argument felt like the most appropriate thing.
| expect(UIManager.measure).toHaveBeenCalledWith( | ||
| expect.any(Number), | ||
| expect.any(Function), | ||
| ); |
There was a problem hiding this comment.
This seems a little janky. Like it's kind of testing internal API details.
There was a problem hiding this comment.
testing the module’s interactions with the public API of its collaborators, rather than the aggregate behavior of said multiple modules, is how you decouple a test like this from implementation details.
from Xunit patterns book:
A Mock Object provides the system under test with both indirect input and a way to verify indirect output. All the expectations are configured before the calling of the method under test.
Asserting that no outbound calls to UIManager.measure happen before the viewRef.measure call is a good pinning test and test as documentation IMO.
There was a problem hiding this comment.
I think my comments were probably swapped. This seems janky and the other one seems like it's monkeying with internals a bit too much for my comfort.
There was a problem hiding this comment.
I'm going to change this to instead have a mock implementation that will throw if passed anything other than a number in the first argument and to call the passed callback with arguments. Then my assertion will just validate that the success callback was called with the expected arguments.
|
Alright, I updated the tests to match the new style we talked about, including the other tests in this file that was the old style. |
We will be changing these functions for Fabric so starting by adding some tests to validate the current behavior.