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

Adding application class for health checking #152

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

sdaschner
Copy link
Contributor

As discussed, a first attempt to add a general "application" type to MST.
Aims to provide an easy access for health checking.
Only MP Health has been added so far (Metrics, OpenAPI, etc., need more sophisticated bindings).
Also provided test examples.

import java.util.ArrayList;
import java.util.List;

public class Health {
Copy link
Member

Choose a reason for hiding this comment

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

I believe this class is the only new class needed and we could remove the MicroShedApplication class entirely. A lot of what the MicroShedApplication does is already done via the ApplicationContainer class (such as knowing hostname, port, and app context root).

If we annotated this class with JAX-RS annotations, then users could inject and use it just like other REST clients from their application. For example:

@MicroShedTest
public class MyServiceTest {

  @Container
  public static ApplicationContainer app = // ...

  @RESTClient
  public static Health healthCheck; // the org.microshed.testing.health.Health instance

  @RESTClient
  public static MyService myService; // some REST endpoint inside the app under test

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So what'd be valuable from my PoV is that folks can programmatically create a class that provides access to the app/system, especially if they can extend it and use it as a nice delegate / abstraction layer to hide complexity. The JAX-RS interfaces kind of go into that direction, but not always fully. Also, I'd love some way to enable health without @MicroShedTest (think of it similar to what RestClientBuilder does).

There's certainly room for adding a solely declarative way, like you're proposing, I also thought about that. However, when is the health check performed, if it's injected here, especially for manual envs? At test startup time? I think that'd make more sense if the user has some interface / "proxy object" to query: "please perform the health check now".


@BeforeEach
void setUp() {
application = MicroShedApplication.withBaseUri("http://localhost:9080").build();
Copy link
Member

Choose a reason for hiding this comment

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

If the application is already running at localhost:9080 that's fine, but we shouldn't hard-code that into the test source. Instead, the host and port should be set with microshed_hostname and microshed_http_port system/env properties and then many sources (such as ApplicationContainer and RestClientBuilder) can auto-detect those values for "local dev" mode, but also switch to running with Testcontainers instances for when this runs in TravisCI.

That last part (also running in CI) is important IMO. If I understand this code correctly, these tests would fail in CI because they require something else to start the server?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, wasn't sure how to fix that, please change so it doesn't interfere with the projects pipeline. In a real-world project I'd also rather make it aware of e.g. System properties or so :)

@aguibert aguibert added the is:enhancement New feature or request label Feb 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
is:enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants