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

Propagate more errors to the client side #458

Merged
merged 2 commits into from
Nov 27, 2023
Merged

Conversation

melix
Copy link
Collaborator

@melix melix commented Nov 23, 2023

This commit improves error reporting on the client side. It propagates some errors which were previously only captured on the server side. In particular, it will now:

  • report which property was being resolved, instead of showing internal auto.test.resources properties
  • capture errors which are sent by the server but were dropped because of lack of JSON error handling
  • report when a container cannot be started or failed during startup
  • provide clearer message when the test resources service is down

Fixes #444

This commit improves error reporting on the client side. It propagates
some errors which were previously only captured on the server side.
In particular, it will now:

- report which property was being resolved, instead of showing
internal `auto.test.resources` properties
- capture errors which are sent by the server but were dropped
because of lack of JSON error handling
- report when a container cannot be started or failed during
startup
- provide clearer message when the test resources service is
down

Fixes #444
@melix melix added the type: improvement A minor improvement to an existing feature label Nov 23, 2023
@melix melix added this to the 2.3.0 milestone Nov 23, 2023
@melix melix self-assigned this Nov 23, 2023
@melix melix requested a review from alvarosanchez November 23, 2023 10:16

@Serdeable
public record SimpleJsonErrorModel(
@JsonProperty("message") String message,
Copy link
Member

Choose a reason for hiding this comment

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

Is the annotation needed here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It shouldn't, kept it for consistency with the other properties

return client.resolve(expression, props, properties);
return withErrorHandling(
() -> client.resolve(expression, props, properties),
() -> "Test resources service wasn't able to revolve expression '" + expression + "'"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
() -> "Test resources service wasn't able to revolve expression '" + expression + "'"
() -> "Test resources service wasn't able to revolve expression '%s'".formatted(expression)

Co-authored-by: Álvaro Sánchez-Mariscal <[email protected]>
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

71.6% 71.6% Coverage
0.0% 0.0% Duplication

@melix melix merged commit 909f3c5 into master Nov 27, 2023
12 checks passed
@melix melix deleted the cc/improve-client-side-errors branch November 27, 2023 09:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: improvement A minor improvement to an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve client side error messages
2 participants