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
Shrohilla/kafka header bindings #308
Conversation
shrohilla
commented
Mar 31, 2022
•
edited
edited
- Added the support for the kafka headers for output bindings
- Reconverting the object from string check if the required params exists than create the KafkaEventData
- It doesn't require the unit test case as the class is internal class
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
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.
Some naming convention issue is there. However generally looks good.
One thing I'd like to recommend is, write unit test. You can use internal scope for enabling unit testing with internalVisuleTo https://codesnippets.fesslersoft.de/use-the-assembly-internalsvisibleto-attribute/
Automated testing seems to fail.
src/Microsoft.Azure.WebJobs.Extensions.Kafka/Output/KafkaProducerAsyncCollector.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Azure.WebJobs.Extensions.Kafka/Output/KafkaProducerAsyncCollector.cs
Show resolved
Hide resolved
src/Microsoft.Azure.WebJobs.Extensions.Kafka/Output/KafkaProducerAsyncCollector.cs
Outdated
Show resolved
Hide resolved
Hi @TsuyoshiUshio I had addressed all the comments shared and created the unit test cases also, please review the code |
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.
The unit tests should include the testing for headers since you added headers feature. You can share the spec and prove it works with variation with writing unit tests.
...Microsoft.Azure.WebJobs.Extensions.Kafka.UnitTests/output/KafkaProducerAsyncCollectorTest.cs
Outdated
Show resolved
Hide resolved
@TsuyoshiUshio -- Already added that in the test -- AddAsync_Item_Is_Of_KafkaEventData_Json_String_Types |
Hi @shrohilla
|
@TsuyoshiUshio Added the few possible validations, will add this scenario in the E2E tests for language workers as a separate task. |