#minor Adding Dynamic Workflows support for SdkTestingExecutor #318
Conversation
|
Thank you for opening this pull request! 🙌 |
rscarvalho
force-pushed
the
rscarvalho/add-dynamic-workflow-testing-support
branch
from
4e5f640 to
054e2ce
Compare
| } | ||
|
|
||
| @Test | ||
| public void testDelegatingWorkflow_OddA() { |
There was a problem hiding this comment.
nit: I think we can keep either OddA or OddB as they are doing the same thing and there isn't much need to have exhaustive tests for of(aConcreteValue % 2 == 0 && bConcreteValue % 2 == 0)) in this case.
|
@rscarvalho Could you please rebase? We fixed the CI issue. |
rscarvalho
force-pushed
the
rscarvalho/add-dynamic-workflow-testing-support
branch
from
7978853 to
32633e1
Compare
|
The last commit was not signed-off. Could you please fix that? |
rscarvalho
force-pushed
the
rscarvalho/add-dynamic-workflow-testing-support
branch
from
32633e1 to
49defc3
Compare
|
There you go, I rebased and signed off all commit! |
Head branch was pushed to by a user without write access
rscarvalho
force-pushed
the
rscarvalho/add-dynamic-workflow-testing-support
branch
from
0ffa3c9 to
1bc7fb4
Compare
| * @return a new {@link SdkTestingExecutor} instance | ||
| * | ||
| * <p>Example usage:</p> | ||
| * <pre>{@code |
There was a problem hiding this comment.
@honnix it seems like there's a conflict between checkstyle's JavadocParagraph module and spotless. When I run mvn verify locally, spotless reformat this snippet to be something like this:
| * @return a new {@link SdkTestingExecutor} instance | |
| * | |
| * <p>Example usage:</p> | |
| * <pre>{@code | |
| * @return a new {@link SdkTestingExecutor} instance | |
| * <p>Example usage: | |
| * <pre>{@code |
But the lack of that empty line before the <p> tag makes checkstyle very unhappy. Should we remove that module from the checkstyle config?
There was a problem hiding this comment.
I will propose a PR to remove checkstyle completely. It has always been creating more troubles than being truly helpful.
There was a problem hiding this comment.
We removed checkstyle in #320 . Could you please remove the last commit and rebase again?
# TL;DR Remove checkstyle maven plugin. ## Type - [ ] Bug Fix - [ ] Feature - [ ] Plugin ## Are all requirements met? - [x] Code completed - [ ] Smoke tested - [ ] Unit tests added - [ ] Code documentation added - [ ] Any pending items have an associated Issue ## Complete description Checkstyle has always been a pain to use and we can hardly recall it really helps with anything. In #318, checkstyle disagrees with spotless, which is very much pointless. ## Tracking Issue _NA_ ## Follow-up issue _NA_ Signed-off-by: Hongxin Liang <[email protected]>
Addding a javadoc to SdkTestingExecutor Signed-off-by: Rodolfo Carvalho <[email protected]>
Signed-off-by: Rodolfo Carvalho <[email protected]>
honnix
force-pushed
the
rscarvalho/add-dynamic-workflow-testing-support
branch
from
1bc7fb4 to
bf04088
Compare
|
Congrats on merging your first pull request! 🎉 |
2 participants
TL;DR
Adds support for testing
SdkDynamicWorkflowTask(dynamic workflows) usingSdkTestingExecutor, enabling assertions on their generated DAGs similarly to regular workflows.Type
Are all requirements met?
Complete description
This PR extends
SdkTestingExecutorto support dynamic workflows (SdkDynamicWorkflowTask) by wrapping them into a lightweight delegating workflow internally. This allows developers to test the structure and outputs of dynamic workflows using the same mechanisms used to test regular workflows, improving consistency and coverage in unit testing Flyte tasks.There were no existing issues or tracking requests for this feature — it was implemented to fill a current gap in testing capabilities for dynamic workflows in
flytekit-testing.Tracking Issue
NA
Follow-up issue
NA