-
Notifications
You must be signed in to change notification settings - Fork 49
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[chrome-api-jstocxxtest] Add function to switch mocks back to recording #1008
Conversation
Continuous Integration: All tests passed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My main question with this CL is why some parts of it are necessary - is there an upcoming CL that uses some mock function that has a record/replay/record/replay pattern - because all I see so far are functions that are record/replay. IIUC the main benefit of #1010 could be achieved if this CL was limited to eliminating the replayAll() from all tests, and adding replay() to all the expect functions.
* @param {string} functionName | ||
* @returns {Function} Mocked function object. | ||
*/ | ||
record(functionName) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was really confused by this name for a bit, because my expectation on reading the name was that this would record an actual function call. Instead, it is more of a reset with extras. I see that you chose record as a name in contrast to replay - but it does behave differently from replay notably in that it returns a mock, while replay is called on a mock and returns void.
What this does is a verify/reset/get. This feels more like a getNewMock() method to me, or maybe getFresh() or getFreshMock(), or maybe just mock() - WDYT?
Also, in your commit message you claim that the testing framework doesn't offer a way to switch between replay and record mode - but isn't this verify/reset exactly that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What this does is a verify/reset/get. This feels more like a getNewMock() method to me, or maybe getFresh() or getFreshMock(), or maybe just mock() - WDYT?
Thanks for the suggestion, updated the name to getFreshMock
.
Also, in your commit message you claim that the testing framework doesn't offer a way to switch between replay and record mode - but isn't this verify/reset exactly that?
Yep, but it's more of a hack probably.
@@ -221,6 +224,7 @@ goog.exportSymbol('testChromeApiProviderToCpp', { | |||
mockControl = new goog.testing.MockControl(); | |||
mockChromeApi = | |||
new MockChromeApi(mockControl, testController.propertyReplacer); | |||
mockControl.$replayAll(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this required? What happens without this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have mocks for addListener/removeListener as well. They need to be switched to replay mode, before listeners get subscribed/unsubscribed. This is happening in ChromeApiProvider constructor/dispose method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about adding replay() calls in the mockChromeApi ctor then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think generally mocks should be in replay
mode during the tests. So if we start putting all of them to replay mode manually it probably can lead to forgetting some of them.
Also, if some mock stays in record mode and gets called accidentally, the errors would be weird, because in record mode the framework will assume that the call is made to set expectation, rather than an actual call.
2c4dc5a
to
e5f6d54
Compare
Continuous Integration: All tests passed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Somehow this CL in its current form feels wrong to me. The testing framework doesn't offer a way to add more expectations when a mock is already in replay mode. I think this is not for some technical reason, but rather because the authors of the framework did not want to include this. This indicates that parts of this CL in its current form are fundamentally considered an anti-pattern by the test framework authors: Specifically, putting everything into replay mode before all (or even most) expectations have been recorded. And the way I see it, this is because the expectations are spread out between the individual tests and the support class, and you are concerned that someone would forget to put mocks of the support class into replay mode.
Now, the way that you want to set up your establishContext helper, it is indeed not possible to use replayAll() just at the time when all expectations have been set, but none of them have been fulfilled. But I think instead of calling it anyway and then reverting it for all test-specific (i.e. interesting) calls, it would be better to introduce a method that calls it for all the observers in the helper class, and use that one instead. Then, the other, more interesting calls would not need to be reset.
To your point that it is possible to forget some of them: This is still possible with the getFreshMock method - whenever it is used, one needs to remember to call replay() on it afterwards. Ultimately, this is a tradeoff of you wanting to put both expectation and execution of a call into the same method in #1010 - I agree this is a good tradeoff though.
And also, whenever using getFreshMock, one basically needs to make sure that there were no expectations on that mock before. I think this is the main reason why I feel this method should not exist - if the verify() part of it actually did something (i.e. an expectation had been recorded and fulfilled), it would be an anti-pattern that makes the actual test hard to understand and follow.
I feel like the intended way was to set all expectations prior to performing any actions on the tested objects. I'm not sure why the authors chose this approach but considering that this framework is quite old, I guess that's the best they came up with. The problem is that it causes the smart card test cases to have quite lengthy descriptions of all the expectations first, and only afterwards, all the calls that are actually giving those results. I find that hard to read. It also creates a lot of copypasted boilerplate. Regarding your suggestion to put replay on other mocks instead of switching test-specific to record mode: I don't know if there are other alternatives. Initially I wanted to extends the StrickMock or FunctionMock somehow. But StrictMock is a final class so it cannot be extended and FunctionMock is not a class, but a function, so it's all very messy :( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#1016 is a good example where this does make sense.
One request though: considering that the getFreshMock function does not use the mock-chrome-api, it should live in the test file chrome-api-jstocxxtest.js instead (or maybe in some other util file if you want to use it in other test files).
Regarding mocks that stay in record mode: Wouldn't the verifyAll() call in the tearDown of the tests catch that?
e5f6d54
to
323223c
Compare
Thanks for reviewing!
Moved to the jstocxxtest
If there would be some calls, yes. |
Continuous Integration: All tests passed. |
Currently all expectations need to be set before any testing takes place, because there's no way to switch between replaying and recording mode in goog.testing framework. Because of that we couldn't factor out repeated pieces of code, such as establishing context, to a separate function.
This CL adds
getFreshMock
function to allow refactoring in the follow-up CLs.