-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
[WIP] Add recipe for hot observables #956
Conversation
Thanks for starting this. Maybe link to https://medium.com/@benlesh/hot-vs-cold-observables-f8094ed53339 for people not knowing the difference. Can you also add it to https://github.com/avajs/ava#recipes
Is there anything AVA could do to better support hot observables? Do you know anyone that could review this? |
@@ -0,0 +1,52 @@ | |||
# How to test hot observables |
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 would name it:
Testing hot observables
The ideal way to test observables with RxJS 5 is actually to use the TestScheduler. However, in a broader sense, support for the The one catch is that unlike promises, sometimes observables are just cancelled, and cancellation does not fire an event for the consumer. That is easy to work around, but the user would need to know what they're doing. |
Yeah, I've dropped the ball on this PR completely. I won't give this up, and will get back once I learn more about testing. My way of doing things is by acting on rxjs like a black box, knowing nothing of how to describe schedules and such. I'm reading a bit of the work on Symbol.observable interopt, but this is way over my head unless I get the proper resources. @Blesh do you believe it would be an antipattern to use above pattern for testing subjects? I wanted to contribute to observable testing, not spread bad sample code :-) |
@marcusnielsen: No I don't think it's an "anti-pattern" but it could result in slow test suites. Also if the source of your hot observable is something non-deterministic it could get hard. That's not usually the case, but it is the reason the TestScheduler exists. The TestScheduler enables synchronous testing of observables by putting all notifications into a virtual scheduler that can be flushed after tests are set up. |
Cool. I'm going to close this for now to clean up the pull request queue, but more than happy to reopen when you're ready for another review :) (Also make sure to address the inline feedback then). |
haha... I just read my previous comment and edited it. I think I dictated it to Google's voice-to-text, because it said "test sweets" instead of "test suites" haha... Sweet tests! |
That's not a typo, that's an effect from using AVA 😎 |
I had to switch to tape due to the lack of feedback on which test was failing, and thus stopping our TDD progress. I wrote this article as a conclusion on the subject. But I still feel the testing style of not using testScheduler in combination with using Subjects might be a bit controventional (although simple to use) for a larger base of RxJS users. |
How is |
I actually swapped from tape/tap to Ava. But we had issues with tests not reporting where the error happened. It went faster to swap back than to find out what was wrong with Ava. This was much clearer to our team of JS devs (3 persons):
The above was better for us compared to the package.json setup with Ava where we didn't know what babel-register even was, what settings it had, and what else Ava was doing behind the doors to transpile our code. It was a bit hard to follow what we had to do to get correct reporting on what row threw the error. We got the transpiled line numbers instead of the ES6 numbers. 2nd, we dealt with a non-closing Socket.io server which meant that about 3 tests were not completing. One thing I love right now with our setup, is that you just import your test files recursively as if it was normal js-code. (I wish the test runner was like reactDom.render and took a bunch of imported tests.) The recursive imports made it easier to find the Socket.io bug since we could comment out whole parts of the file-tree until the bug went away. I wish I had more time to stick with different products, but we have the investor's stress right now. Once we get into a more stable phase with our product I hope we can help the OSS world more. I'd love to see Ava succeed, but I'd love to get a more slimmed experience where transpilation isn't a part of the tool. And also get some kind of feedback on what tests are done, and which are pending/timed out. I really hope the information above will help in some way. Helping is hard! |
Thanks for the feedback @marcusnielsen. Appreciate you taking the time to write this out and totally understandable.
I think using different globs could have helped here, like only testing a sub-directory:
That's indeed annoying. Has happened to me too. I have some ideas for using https://github.com/mafintosh/why-is-node-running to show why it's pending, but in the short term, we plan on showing pending tests: #583
|
Cool! I'm happy to give feedback to anyone if it helps. Glob patterns is something I just don't benefit from. An aside (but not vital) is that we run docker-compose, and executing special commands is somewhat more complex. |
I think it would be nice to add some recipies for observables. Right now, I'm not able to use the observable snippet in the readme since it is based on cold observables. I made a draft of how I currently do testing with hot observables using Subjects. But since I'm new at observables, someone more senior should go in and say if this style is valid. There might be some even better way that I haven't learned about yet.
Also, it would be nice to just note that the readme docs example is only for hot observables. It might confuse newbies (like it did for me) and be a letdown when you discover that Ava doesn't support most of the observables created in web development.
I just wrote this code from my head. If you want to accept this recipe, I will go through the code and test run it before a possible merge.