Skip to content

Convert hystrix 1.4 test from groovy to java#9217

Merged
laurit merged 9 commits into
open-telemetry:mainfrom
jaydeluca:convert-hystrix-1-4-tests
Aug 18, 2023
Merged

Convert hystrix 1.4 test from groovy to java#9217
laurit merged 9 commits into
open-telemetry:mainfrom
jaydeluca:convert-hystrix-1-4-tests

Conversation

@jaydeluca

Copy link
Copy Markdown
Member

Related to #7195

@jaydeluca jaydeluca requested a review from a team August 16, 2023 12:45
testing.waitAndAssertTraces(
trace ->
trace.hasSpansSatisfyingExactly(
span -> span.hasName("parent").hasNoParent(),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

groovy code also verified that there are no attributes for which in java tests we usually use .hasAttributes(Attributes.empty())

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

thank you for pointing this out, i'll be sure to include these going forward

span.hasName("ExampleGroup.HystrixTest$1.execute")
.hasParent(trace.getSpan(0))
.hasAttributesSatisfyingExactly(
equalTo(stringKey("hystrix.command"), "HystrixTest$1"),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

IDK whether this is worth the effort but instead of anonymous classes could use method local classes, something like

    class TestCommand extends HystrixCommand<String> {

      protected TestCommand(Setter setter) {
        super(setter);
      }

      @Override
      protected String run() throws Exception {
        return tracedMethod();
      }

      private String tracedMethod() {
        testing.runWithSpan("tracedMethod", () -> {});
        return "Hello!";
      }
    }

    HystrixCommand<String> command = new TestCommand(setter());

then here this assertion would be equalTo(stringKey("hystrix.command"), "TestCommand") Then for each test in the same file would need to use a different name for the command to avoid $x in the class name.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

great idea, thank you!

};

String result = testing.runWithSpan("parent", () -> operation.apply(command));
assertThat(Objects.equals(result, "Hello!")).isTrue();

@laurit laurit Aug 17, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

To me using Object.equals here doesn't make sense, try assertThat(result).isEqualTo("Hello!");

Comment on lines +262 to +269
.map(
error -> {
if (error instanceof Throwable) {
return ((Throwable) error).getMessage();
} else {
throw new IllegalStateException("Expected Throwable result");
}
})

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is a new addition, not present in the original groovy test. Is this used?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

when I was writing the test I had some return type errors for this method and I wrote this to get around them, but now when I remove this code I no longer see those issues so I must have had another unrelated issue at the time that I have since resolved. I have removed the unneeded code from this PR

Comment on lines +426 to +435
.hasEventsSatisfyingExactly(
event ->
event
.hasName("exception")
.hasAttributesSatisfyingExactly(
equalTo(
EXCEPTION_TYPE, "java.lang.IllegalArgumentException"),
satisfies(
EXCEPTION_STACKTRACE,
val -> val.isInstanceOf(String.class))))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
.hasEventsSatisfyingExactly(
event ->
event
.hasName("exception")
.hasAttributesSatisfyingExactly(
equalTo(
EXCEPTION_TYPE, "java.lang.IllegalArgumentException"),
satisfies(
EXCEPTION_STACKTRACE,
val -> val.isInstanceOf(String.class))))
.hasException(exception.getCause())

Comment on lines +408 to +421
.hasEventsSatisfyingExactly(
event ->
event
.hasName("exception")
.hasAttributesSatisfyingExactly(
satisfies(
EXCEPTION_STACKTRACE,
val -> val.isInstanceOf(String.class)),
equalTo(
EXCEPTION_TYPE,
"com.netflix.hystrix.exception.HystrixRuntimeException"),
equalTo(
EXCEPTION_MESSAGE,
"TestCommand failed and no fallback available."))),

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
.hasEventsSatisfyingExactly(
event ->
event
.hasName("exception")
.hasAttributesSatisfyingExactly(
satisfies(
EXCEPTION_STACKTRACE,
val -> val.isInstanceOf(String.class)),
equalTo(
EXCEPTION_TYPE,
"com.netflix.hystrix.exception.HystrixRuntimeException"),
equalTo(
EXCEPTION_MESSAGE,
"TestCommand failed and no fallback available."))),
.hasException(exception),

@jaydeluca jaydeluca force-pushed the convert-hystrix-1-4-tests branch from ef68787 to 4bb7b9b Compare August 18, 2023 10:10

@mateuszrzeszutek mateuszrzeszutek left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks @jaydeluca !

@laurit laurit merged commit cb0d00a into open-telemetry:main Aug 18, 2023
@jaydeluca jaydeluca deleted the convert-hystrix-1-4-tests branch September 24, 2023 15:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants