thbar/kiba

Misleading test

vfonic opened this issue · 5 comments

vfonic commented

I just found this test class and I think it might be misleading, not doing what it's described to do:
https://github.com/thbar/kiba/blob/a2b5573f49857e57bd3739b908587fa5539ba8f1/test/test_buffering_transform.rb

It's describing Buffering Transform, but it's actually using Streaming Transform.

vfonic commented

I also noticed that you use this transformer in StreamingRunner, but it doesn't implement #process method:
https://github.com/thbar/kiba/blob/d08b311079b63a99d0275b44e108a305d06e5753/test/support/test_enumerable_source.rb

https://github.com/thbar/kiba/blob/d08b311079b63a99d0275b44e108a305d06e5753/test/test_streaming_runner.rb

Does it still work? Is this the correct syntax?

thbar commented

Thanks for the feedback! Let me clarify point by point:

1/ test_buffering_transform intent is not to test the streaming runner (which is tested in TestStreamingRunner) but the fact that a transform calling close should be allowed to yield.

The intent is obviously unclear, so I'll refactor and will probably group the 2 tests in the same test class, with a better phrasing. Thanks!

2/ test_enumerable_source is not a transformer, but a source! That's why it only implements the constructor and #each.

The syntax hasn't changed - you still have:

  • Sources must implement #each and yield records
  • Transforms must implement #process, can optionally yield records from there and must return one record (optionally nil), and can optionally implement #close (which can yield too)
  • Destinations must implement #write, can optionally implement #close

@vfonic hope this clarify things a bit! Let me know otherwise, happy to improve the tests.

thbar commented

@vfonic I went ahead and attempted to improve the situation with #63.

Let me know if this is clearer!

I also just noticed you improved the documentation about that. Many thanks, I'll definitely make a second pass in the future to ensure everything is clear here!

vfonic commented

Yeah, perfectly clear now. I had a (completely unrelated to this issue) demo that was approaching. 😇 It was also the first time I needed to use StreamingRunner. I made some changes in a piece of the code that I didn't have the tests for and I just assumed that there's some issue with StreamingRunner as everything else seemed to be working. 😆 I've found the bug and fixed it and StreamingRunner worked as expected. :)
Thanks for the quick response!

thbar-ci-aojf3z commented