Add build_csv_enumerator to the EnumeratorBuilder#183
Conversation
basicBrogrammer
force-pushed
the
jw/add-build-csv-to-enumerator-builder
branch
from
2965ecd to
c02247d
Compare
basicBrogrammer
force-pushed
the
jw/add-build-csv-to-enumerator-builder
branch
from
c02247d to
f594bb3
Compare
basicBrogrammer
requested review from
adrianna-chang-shopify,
etiennebarrie,
pedropb and
rporrasluc
and removed request for
etiennebarrie
|
|
||
| test_builder_method(:build_csv_enumerator) do | ||
| enum = enumerator_builder(wraps: 0).build_csv_enumerator(CSV.new("test"), cursor: nil) | ||
| assert_instance_of(Enumerator::Lazy, enum) |
There was a problem hiding this comment.
Does this assertion really belong here, with the way this test file is set up?
There was a problem hiding this comment.
I wanted to validate that JobIteration::CsvEnumerator is being used was being used. Do you think I should add it as a separate test?
There was a problem hiding this comment.
Probably. This test_builder_method method should return the enumerator itself, as it generates a test through meta-programming. If you want to test something in addition, adding a separate test seems better.
There was a problem hiding this comment.
The EnumeratorBuilder methods are straightforward enough that you may not need a specific assertion. I'm not sure what this one really brings us for example, a Lazy Enumerator could be anything really. In any case if you really care about asserting this, another test would be preferable, those are really about the wrapping feature.
test_builder_methodmethod should return the enumerator itself
Technically since it sets up a mocha expectation that is just expected to be triggered during the block, we don't really care about the return value, it does check that this enumerator isn't wrapped.
rporrasluc
left a comment
rporrasluc
left a comment
There was a problem hiding this comment.
Once @Mangara's comment is addressed this looks great!
| test "#build_csv_enumerator uses the CsvEnumerator class" do | ||
| csv = CSV.new('test') | ||
| expected_cursor = 42 | ||
| enumerator = JobIteration::CsvEnumerator.new(csv) | ||
| builder = EnumeratorBuilder.new(mock, wrapper: mock) | ||
|
|
||
| JobIteration::CsvEnumerator.expects(:new).with(csv).returns(enumerator) | ||
| enumerator.expects(:rows).with(cursor: expected_cursor) | ||
| builder.build_csv_enumerator(csv, cursor: expected_cursor) | ||
| end |
There was a problem hiding this comment.
@Mangara & @etiennebarrie How about this?
There was a problem hiding this comment.
Usually I don't like those tests as they just duplicate the code in mocks.
There was a problem hiding this comment.
@etiennebarrie How about the new test? It's kind of copy pasta of a csv enum test but if we don't want to use the mock this is the only way I can think of to verify the output of #build_csv_enumerator
basicBrogrammer
force-pushed
the
jw/add-build-csv-to-enumerator-builder
branch
from
bd9a295 to
dc76741
Compare
basicBrogrammer
force-pushed
the
jw/add-build-csv-to-enumerator-builder
branch
from
dc76741 to
83b32b2
Compare
5 participants
The Billing team is working towards adding the job-iteration gem and removing the legacy
BackgroundQueue::Iteration. To achieve feature parity, we can addbuild_csv_enumeratorto the EnumeratorBuilder. Thebuild_throttle_enumeratorand others gives us precedence to add this method as well.