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.

Sign up for GitHub

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

Jump to bottom

Describe known inputs when input type not found in TestingRunnableNode #257

Merged
merged 1 commit into from
Oct 18, 2023
Merged

Describe known inputs when input type not found in TestingRunnableNode #257

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 23 additions & 4 deletions flytekit-testing/src/main/java/org/flyte/flytekit/testing/TestingRunnableNode.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,9 @@

import java.util.HashMap;
import java.util.Map;
import java.util.Objects;
import java.util.function.Function;
import java.util.stream.Collectors;
import org.flyte.api.v1.Literal;
import org.flyte.api.v1.PartialIdentifier;
import org.flyte.api.v1.RunnableNode;
Expand Down Expand Up @@ -84,7 +86,7 @@ protected TestingRunnableNode(
public Map<String, Literal> run(Map<String, Literal> inputs) {
InputT input = inputType.fromLiteralMap(inputs);

if (fixedOutputs.size() == 0) {
if (fixedOutputs.isEmpty()) {
// No mocking via input matching, either run the real thing or run the provided lambda
if (runFn != null) {
return outputType.toLiteralMap(runFn.apply(input));
Expand All @@ -101,14 +103,31 @@ public Map<String, Literal> run(Map<String, Literal> inputs) {

String message =
String.format(
"Can't find input %s for remote %s [%s] across known %s inputs, "
+ "use %s to provide a test double",
input, type, getName(), type, testingSuggestion);
"Can't find input for remote %s [%s] across known %s inputs.%s",
type, getName(), type, System.lineSeparator())
+ String.format(
"Input: %s%sKnown inputs: %s%sUse %s to provide a test double",
input,
System.lineSeparator(),
knownInputsString(),
System.lineSeparator(),
testingSuggestion);

// Not matching inputs and there is nothing to run
throw new IllegalArgumentException(message);
}

private String knownInputsString() {
Copy link
Contributor

@AndersonReyes AndersonReyes Oct 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just being nosy because I want this improvement as well but I wonder if we should filter the fixedOutputs for keys of type InputT input above. to noise? Specially for tasks with many inputs. If the mocks is wrong you’ll have to potentially skim through many inputs in the error message to see if it’s missing or the mocked parameters are wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @AndersonReyes thanks for the comment. Im afraid my understanding of flytekit is limited to my use of it, I'm not quite understanding what you mean here. Could you perhaps outline an example and I will try to create a test for it when I have a little time?

Copy link
Contributor

@AndersonReyes AndersonReyes Oct 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually ignore me you are already doing it. For some reason I thought fixedOutputs here was all the mocked inputs for the entire workflow (and that’s why I said maybe we need filter) but it’s not. fixedOutputs is just for this runnable node which makes more sense but ignore me in comment above

String knownInputs =
fixedOutputs.keySet().stream()
.map(Objects::toString)
.collect(Collectors.joining(System.lineSeparator()));
if (knownInputs.isEmpty()) {
return "{}";
}
return knownInputs;
}

@Override
public String getName() {
return id.name();
Expand Down
15 changes: 12 additions & 3 deletions flytekit-testing/src/test/java/org/flyte/flytekit/testing/SdkTestingExecutorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,7 +253,10 @@ public Void expand(SdkWorkflowBuilder builder, Void noInput) {
assertThat(
e.getMessage(),
equalTo(
"Can't find input RemoteSumInput{a=SdkBindingData{type=integers, value=1}, b=SdkBindingData{type=integers, value=2}} for remote task [remote_sum_task] across known task inputs, use SdkTestingExecutor#withTaskOutput or SdkTestingExecutor#withTask to provide a test double"));
"Can't find input for remote task [remote_sum_task] across known task inputs.\n"
+ "Input: RemoteSumInput{a=SdkBindingData{type=integers, value=1}, b=SdkBindingData{type=integers, value=2}}\n"
+ "Known inputs: RemoteSumInput{a=SdkBindingData{type=integers, value=10}, b=SdkBindingData{type=integers, value=20}}\n"
+ "Use SdkTestingExecutor#withTaskOutput or SdkTestingExecutor#withTask to provide a test double"));
}

@Test
Expand Down Expand Up @@ -383,7 +386,10 @@ public TestUnaryIntegerIO expand(SdkWorkflowBuilder builder, SumLaunchPlanInput
assertThat(
ex.getMessage(),
equalTo(
"Can't find input SumLaunchPlanInput{a=SdkBindingData{type=integers, value=3}, b=SdkBindingData{type=integers, value=5}} for remote launch plan [SumWorkflow] across known launch plan inputs, use SdkTestingExecutor#withLaunchPlanOutput or SdkTestingExecutor#withLaunchPlan to provide a test double"));
"Can't find input for remote launch plan [SumWorkflow] across known launch plan inputs."
+ "\nInput: SumLaunchPlanInput{a=SdkBindingData{type=integers, value=3}, b=SdkBindingData{type=integers, value=5}}"
+ "\nKnown inputs: SumLaunchPlanInput{a=SdkBindingData{type=integers, value=100000}, b=SdkBindingData{type=integers, value=100000}}"
+ "\nUse SdkTestingExecutor#withLaunchPlanOutput or SdkTestingExecutor#withLaunchPlan to provide a test double"));
}

@Test
Expand Down Expand Up @@ -504,7 +510,10 @@ public Void expand(SdkWorkflowBuilder builder, Void noInput) {
assertThat(
e.getMessage(),
equalTo(
"Can't find input RemoteSumInput{a=SdkBindingData{type=integers, value=1}, b=SdkBindingData{type=integers, value=2}} for remote task [remote_sum_task] across known task inputs, use SdkTestingExecutor#withTaskOutput or SdkTestingExecutor#withTask to provide a test double"));
"Can't find input for remote task [remote_sum_task] across known task inputs.\n"
+ "Input: RemoteSumInput{a=SdkBindingData{type=integers, value=1}, b=SdkBindingData{type=integers, value=2}}\n"
+ "Known inputs: RemoteSumInput{a=SdkBindingData{type=integers, value=10}, b=SdkBindingData{type=integers, value=20}}\n"
+ "Use SdkTestingExecutor#withTaskOutput or SdkTestingExecutor#withTask to provide a test double"));
}

public static class SimpleUberWorkflow
Expand Down
26 changes: 20 additions & 6 deletions flytekit-testing/src/test/java/org/flyte/flytekit/testing/TestingRunnableNodeTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.hasEntry;
import static org.hamcrest.Matchers.oneOf;
import static org.junit.jupiter.api.Assertions.assertThrows;

import com.google.auto.value.AutoValue;
Expand Down Expand Up @@ -70,7 +71,10 @@ void testRun_preferStubValuesOverFunction() {

@Test
void testRun_notFound() {
Map<Input, Output> fixedOutputs = singletonMap(Input.create("7"), Output.create(7L));
Map<Input, Output> fixedOutputs =
Map.of(
Input.create("7"), Output.create(7L),
Input.create("8"), Output.create(8L));
TestNode node = new TestNode(null, fixedOutputs);

IllegalArgumentException ex =
Expand All @@ -80,9 +84,17 @@ void testRun_notFound() {

assertThat(
ex.getMessage(),
equalTo(
"Can't find input Input{in=SdkBindingData{type=strings, value=not in fixed outputs}} for remote test [TestTask] "
+ "across known test inputs, use a magic wang to provide a test double"));
oneOf(
"Can't find input for remote test [TestTask] across known test inputs.\n"
+ "Input: Input{in=SdkBindingData{type=strings, value=not in fixed outputs}}\n"
+ "Known inputs: Input{in=SdkBindingData{type=strings, value=8}}\n"
+ "Input{in=SdkBindingData{type=strings, value=7}}\n"
+ "Use a magic wang to provide a test double",
"Can't find input for remote test [TestTask] across known test inputs.\n"
+ "Input: Input{in=SdkBindingData{type=strings, value=not in fixed outputs}}\n"
+ "Known inputs: Input{in=SdkBindingData{type=strings, value=7}}\n"
+ "Input{in=SdkBindingData{type=strings, value=8}}\n"
+ "Use a magic wang to provide a test double"));
}

@Test
Expand Down Expand Up @@ -128,8 +140,10 @@ void testWithoutRunFn() {
assertThat(
ex.getMessage(),
equalTo(
"Can't find input Input{in=SdkBindingData{type=strings, value=7}} for remote test [TestTask] "
+ "across known test inputs, use a magic wang to provide a test double"));
"Can't find input for remote test [TestTask] across known test inputs.\n"
+ "Input: Input{in=SdkBindingData{type=strings, value=7}}\n"
+ "Known inputs: {}\n"
+ "Use a magic wang to provide a test double"));
}

@Test
Expand Down
Loading