-
Notifications
You must be signed in to change notification settings - Fork 21
feat(real-time): add real-time package #3023
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
Conversation
Add real-time package. It is a helper package to help with the nodejs side of a real-time report. It abstracts away all the nitty gritty server-send events stuff underneath.
BundleMon (elements)Unchanged files (3)
No change in files bundle size Final result: ✅ View report in BundleMon website ➡️ |
| let onCompleteStub: sinon.SinonStub<[], void>; | ||
|
|
||
| beforeEach(() => { | ||
| responseMock = sinon.createStubInstance(ServerResponse); |
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.
It seems a bit strange to me to make a stub instance for something like a http response. Is it not possible to make the object ourselves and spy on it? Especially since the sut is doing quite a lot of side-effect calling on the response this doesn't give me faith that it is being used properly
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.
It is a unit test. I want to avoid creating an actual response.
Do you mean to say it's better to write a custom fake that implements the ServerResponse interface?
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.
Well, I expected ServerResponse to just be a data object carrying some stream objects or something. That way you could test the streams. If that's not the case, this might be the simplest approach. Then a custom fake doesn't have much advantage over a stub
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.
Hmm I understand. But it is more complex than a data object. The docs give a hint:
This object is created internally by an HTTP server, not by the user. It is passed as the second parameter to the 'request' event.
We also cannot depend on a class higher up, since we are using the writeHead method, which is implemented on the ServerResponse class directly.
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 clearly expected too much 🙃
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.
Haven't you learned by now? This is JavaScript after all 😉
xandervedder
left a comment
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.
Looking forward to use this in stryker-dashboard! 🥳
|
@hugo-vrijswijk I've commented on both remarks you've made. Could you take a look and answer my questions? |
hugo-vrijswijk
left a comment
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.
@nicojs not sure if you were waiting for specific approval from my side but this looks good 👍
Add real-time package. It is a helper package to help with the nodejs side of a real-time report. It abstracts away all the nitty gritty server-send events stuff underneath.