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

conjure assertions provide diagnostic info for unexpected exceptions #883

Merged
merged 3 commits into from
Aug 9, 2022

Conversation

carterkozak
Copy link
Contributor

This is based on the idea provided by #882, implemented to use
standard assertj types and provide feedback consistent with builtin
assertj checks.

==COMMIT_MSG==
conjure assertions provide diagnostic info for unexpected exceptions
==COMMIT_MSG==

This is based on the idea provided by #882, implemented to use
standard assertj types and provide feedback consistent with builtin
assertj checks.
@changelog-app
Copy link

changelog-app bot commented Aug 8, 2022

Generate changelog in changelog/@unreleased

Type
See change types. Select one:

  • Feature
  • Improvement
  • Fix
  • Break
  • Deprecation
  • Manual task
  • Migration

Description

conjure assertions provide diagnostic info for unexpected exceptions

Check the box to generate changelog(s)

  • Generate changelog entry

@@ -73,7 +73,7 @@ private void failIfNotEqual(String message, Object expected, Object actual) {
public final ServiceExceptionAssert containsArgs(Arg<?>... args) {
isNotNull();

AssertableArgs actualArgs = new AssertableArgs(actual.getParameters());
AssertableArgs actualArgs = new AssertableArgs(actual.getArgs());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Migrated away from deprecated methods

.hasMessage("Expecting code to throw a com.palantir.conjure.java.api.errors.ServiceException,"
+ " but caught a java.lang.RuntimeException.");
.hasMessageContaining(
"com.palantir.conjure.java.api.errors.ServiceException", "java.lang.RuntimeException");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Generified the asserts since it's not generally helpful to test precise string matches.

Copy link

Choose a reason for hiding this comment

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

I don't care about the precise string, but maybe to test that we are serializing the underlying error messages:

    assertThatThrownBy(() -> assertThatServiceExceptionThrownBy(() -> {
                throw new RuntimeException("My helpful message");
            }))
            .hasMessageContaining(
                    "com.palantir.conjure.java.api.errors.ServiceException",
                    "java.lang.RuntimeException",
                    "My helpful message");

Choose a reason for hiding this comment

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

+1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

Copy link

@jkozlowski jkozlowski left a comment

Choose a reason for hiding this comment

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

I would support checking that the full message is serialized, as that's the point of this change, but I don't know the assertj implementation. If that's what it's basically supposed to do, then let's ship it, no need to re-test their implementation.

@bulldozer-bot bulldozer-bot bot merged commit 760e7e2 into develop Aug 9, 2022
@bulldozer-bot bulldozer-bot bot deleted the ckozak/assertion_cause branch August 9, 2022 13:53
@svc-autorelease
Copy link
Collaborator

Released 2.28.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants