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

Allow annotations to be explicitly enabled via config #275

Merged
merged 5 commits into from
May 15, 2018

Conversation

Azquelt
Copy link
Member

@Azquelt Azquelt commented May 11, 2018

Allow annotations to be explicitly enabled via config as well as being disabled.

This allows a Fault Tolerance annotation to be disabled globally, but then enabled for a specific class or method.

Add TCKs to test this behaviour.

This is to address #266

This includes and completes the work that @Emily-Jiang started under #270

@Azquelt
Copy link
Member Author

Azquelt commented May 11, 2018

The last commit is unrelated but is needed to allow the project to build.

Fixed elsewhere, no longer needed.

Copy link
Member

@Emily-Jiang Emily-Jiang left a comment

Choose a reason for hiding this comment

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

Can you add more tests to test each annotation?

Emily-Jiang and others added 5 commits May 12, 2018 17:27
Signed-off-by: Andrew Rouse <[email protected]>
This is to allow TestException to be used in other tests.

Also refactor how the metrics tests include util packages to simplify
the Deployment methods.

Signed-off-by: Andrew Rouse <[email protected]>
* Simplify existing tests.
* Add new tests to check that method config overrides class config and
class config overrides global config
* Add tests for Bulkhead which were previously missing

Also changes tests from throwing RuntimeException to throwing
TestException. Some of the old tests would incorrectly catch
FaultToleranceExceptions when the intent was to only catch the
RuntimeException thrown by the test method.

Signed-off-by: Andrew Rouse <[email protected]>
@Azquelt
Copy link
Member Author

Azquelt commented May 12, 2018

This ended up being a fairly large change. There are several different ways that config parameters can override each other and testing them across all the annotations makes for a fairly large test matrix.

Given that we want to run very similar tests in lots of different configurations, I've written a new "client" bean with behaviour designed to make it very easy to test whether or not the annotations are enabled.

This allows the actual test methods to be a few lines in most cases. (The exception is the Bulkhead tests because bulkheads need several executions running in parallel and this takes a few more lines to set up.)

The methods on the new client bean throw TestException as where the old client bean threw a RuntimeException. This was because I found that the old circuitBreaker enablement tests were assuming that a caught RuntimeException was thrown from the test bean, whereas it could actually be a CircuitBreakerOpenException as that's a subclass of RuntimeException. Using a separate exception class to act as a dummy application exception avoids the potential for this sort of mistake.

I've added bulkhead enablement tests which were previously missing.

Note: I'm slightly unsure about the bulkhead tests. I'm not sure it's guaranteed that the ApplicationScoped context is active on the new thread, but I went with it since that's the approach we already use in BulkheadSynchTest.

I've refactored the way the config is declared to try to minimise copy paste errors since the meat of these tests is setting up different configurations.

Copy link
Member

@Emily-Jiang Emily-Jiang left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks @Azquelt !

@Emily-Jiang Emily-Jiang merged commit 9be1093 into eclipse:master May 15, 2018
@Azquelt Azquelt deleted the allow-enable-with-config branch November 27, 2018 16:44
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.

2 participants