-
Notifications
You must be signed in to change notification settings - Fork 49
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
misc: add rules engine codegen tests #1459
base: main
Are you sure you want to change the base?
Conversation
A new generated diff is ready to view. |
A new generated diff is ready to view. |
A new generated diff is ready to view. |
A new generated diff is ready to view. |
This comment has been minimized.
This comment has been minimized.
settings.gradle.kts
Outdated
@@ -53,6 +53,7 @@ include(":hll:hll-mapping-core") | |||
include(":services") | |||
include(":tests") | |||
include(":tests:codegen:event-stream") | |||
include("tests:codegen:rules-engine") |
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.
correctness: missing leading colon :tests:codegen:rules-engine
// doing it this way (as opposed to doing what we do for protocol-tests) allows | ||
// the tests to work without a publish to maven-local step at the cost of maintaining | ||
// this set of dependencies manually | ||
// <-- BEGIN GENERATED DEPENDENCY LIST --> |
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.
What is this dependency list generated by?
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.
I think it's just manually written. I couldn't find anything that suggests it's code generated
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.
I noticed most of this file is copy-pasted from elsewhere, is there any reason this needs to be a new module rather than in the existing codegen tests?
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.
Organization mostly, it didn't feel right to have this under tests/codegen/event-stream
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.
I'd rather see the codegen build scripts abstracted to cover both event streams and endpoint rules than copy-pasting the entire file, is that possible?
Quality Gate passedIssues Measures |
A new generated diff is ready to view.
|
Affected ArtifactsChanged in size
|
Issue #
Description of changes
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.