Misleading test - Githubissues

thbar / kiba

Data processing & ETL framework for Ruby

https://www.kiba-etl.org

Other

1.75k stars 87 forks source link

Misleading test #62

Closed vfonic closed 5 years ago

vfonic commented 5 years ago

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 5 years ago

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 5 years ago

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 5 years ago

@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 5 years ago

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 5 years ago

Excellent! Glad it’s working. I wish you a great demo!

Envoyé de mon iPhone

Le 6 nov. 2018 à 22:28, Viktor Fonic notifications@github.com a écrit :

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!

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.