Skip to content
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

test(#2374): provide a framework to easily unit test processing elements #2730

Conversation

IsaakKrut
Copy link
Contributor

Added a couple of features to the test executor.

  1. Option to provide custom configuration. TestComposeProcessor required to set output strategies and TestMergeByTimeProcessor required to set output key selectors.
  2. Mocked mockParams.getModel(). Required for processors of type StreamPipesDataProcessor, which generate their own extractor based on the graph provided
  3. Updated event creation to detect the prefix in the map keys and parse it properly

Refactored all tests in streampipes-processors-filters-jvm to use the executor

PR introduces (a) breaking change(s): no

PR introduces (a) deprecation(s): no

@github-actions github-actions bot added java Pull requests that update Java code pipeline elements Relates to pipeline elements backend Everything that is related to the StreamPipes backend testing Relates to any kind of test (unit test, integration, or E2E test). labels Apr 12, 2024
@bossenti bossenti changed the title 2374 provide a framework to easily unit test processing elements test(#2374): provide a framework to easily unit test processing elements Apr 15, 2024
@bossenti bossenti linked an issue Apr 15, 2024 that may be closed by this pull request
@tenthe
Copy link
Contributor

tenthe commented Apr 15, 2024

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

@IsaakKrut
Copy link
Contributor Author

@tenthe Suggestions make sense to me. Made the updates

@tenthe
Copy link
Contributor

tenthe commented Apr 16, 2024

@IsaakKrut, thanks for the changes.
I'll merge this PR.
As a next step, my suggestion would be to move the ProcessingElementTestExecutor into the module streampipes-test-utils. You can add a new package there.

Furthermore, I would suggest to write a builder to create the userConfigurations. Within this builder we will hopefully be able to 'hide' the prefix information of event streams.

@IsaakKrut, do you want to provide a suggestion for the design of such an API?
Do you want to create an issue for this or shoud I do that?

@tenthe tenthe merged commit 64d2b94 into apache:2374-provide-a-framework-to-easily-unit-test-processing-elements Apr 16, 2024
21 checks passed
@IsaakKrut
Copy link
Contributor Author

@tenthe I created issue #2737. Could you assign it to me?

tenthe added a commit that referenced this pull request May 2, 2024
* feat(#2374): Add suggestion to test processing elements

* #2374 refactored unit tests to use ProcessingElementTestExecutor

* #2374 cleanup

* #2374 cleanup

* #2374 updated test executor to non-static methods

* #2374 updated test executor to non-static methods

* test(#2374): provide a framework to easily unit test processing elements (#2730)

* #2374 refactored unit tests to use ProcessingElementTestExecutor

* #2374 cleanup

* #2374 cleanup

* #2374 updated test executor to non-static methods

* #2374 updated test executor to non-static methods

* #2374 Added test configuration builder and resolved selector prefixes

* #2374 Moved test executor to a new package

* feat(#2374): Fix cyclic dependencies and checkstyle

* feat(#2374): Fix checkstyle

* #2374 extracted TestConfigurationBuilder into a separate file

---------

Co-authored-by: Isaak <krutisaak099@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend Everything that is related to the StreamPipes backend java Pull requests that update Java code pipeline elements Relates to pipeline elements testing Relates to any kind of test (unit test, integration, or E2E test).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide a framework to easily unit test processing elements
2 participants