tenthe commented on PR #2730: URL: https://github.com/apache/streampipes/pull/2730#issuecomment-2056556959
Thanks a lot for the PR and updating the old tests. I really appreciate your effort in improving the testing framework step by step. While reviewing the current version, I have some minor comments that I think could enhance the overall clarity and cleanliness of the codebase: 1. Regarding the `ProcessingElementTestExecutor`, I'm not entirely convinced if adding a separate method to validate an exception is thrown is the best approach. Instead, I suggest that the test asserts that an exception is thrown. This adjustment would make the API cleaner, as it would prevent the `expectedOutputEvents` and `invocationConfigs` parameters from consistently being empty or null. What are your thoughts on this? 2. Additionally, I believe we can make some improvements to the API of `ProcessingElementTestExecutor`. Currently, all methods are static. My suggestion is to make these variables non-static by passing them into the constructor. This approach would allow us to provide different constructors if certain parameters must not be set. Specifically, I propose adding `IStreamPipesDataProcessor processor`, `Map<String, Object> userConfiguration`, and `Consumer<DataProcessorInvocation> invocationConfig` as constructor parameters. Consequently, the `run` method's signature would be updated to accept `List<Map<String, Object>> inputEvents` and `List<Map<String, Object>> expectedOutputEvents`. What do you think of that? Please feel free to respond to the comments, especially if you have a different opinion. As soon as we have cleaned up the API a bit, we can think about how to remove the stream prefixes from the tests. These currently make the tests somewhat difficult to read -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
