Setting a first test foundation for Metrics event#1439
Setting a first test foundation for Metrics event#1439
Conversation
|
| public ExecutorService getExecutorService() { | ||
| return service; | ||
| } |
There was a problem hiding this comment.
undo, this won't be needed (see other comment why)
| import static org.assertj.core.api.Assertions.within; | ||
| import static org.junit.jupiter.api.Assertions.fail; | ||
|
|
||
| final class MetricsTests { |
There was a problem hiding this comment.
this test is too much in terms of complexity for no tangible benefit.
lets step back and create something simpler that beginners can understand and maintain while providing pretty much the same test-safety in terms of relevant coverage.
- create a package private method in Metrics called
countBlocking. its identical tocountbut it won't use the executor service. so it creates the instant and then callsprocessEventdirectly - in ur unit test this will be the method ur testing against now. without the async stuff the test will simplify a lot
- the test should use a GWT structure (see other examples in the codebase, alternative patterns that share the same idea: 4-phases SEVT or AAA). try to aim for something like this:
@Test
void basicEventCount() {
// GIVEN a test event
String expectedEvent = "test";
Instant expectedHappenedAt = Instant.now();
// WHEN counting the event
metrics.countBlocking(expectedEvent);
// THEN the event was saved in the DB
Foo entry = database...
String actualEvent = entry.event();
Instant actualHappenedAt = entry.happenedAt();
assertEquals(expectedEvent, actualEvent);
assertCloseEnough(expectedHappenedAt, actualHappenedAt);
}
private static void assertCloseEnough(Instant expected, Instant actual) {
... // assert that its within 1min difference
} (double check the correct order of expected vs actual in assert, i never can remember it, lol)
There was a problem hiding this comment.
Thanks for the guide. However, i don't see this a good idea: creating new method inside the Metrics, while it's only for tests. Also putting such method in the test class, doesn't really test the real one count
There was a problem hiding this comment.
It is not meaningful to test the true count method. The async nature makes it very difficult to test, too difficult for what we want right now. It is more meaningful to instead split the method into parts so that your test can at least cover 98% of its code. Thats good and also standard practice.
Please do it, thanks.
There was a problem hiding this comment.
Why do we have to create an assertion method assertCloseEnough ?
|
| /** | ||
| * Track an event execution with flag for async execution. | ||
| * | ||
| * @param event the event to save | ||
| * @param doAsync the async flag | ||
| */ | ||
| public void count(String event, boolean doAsync) { | ||
| count(event, Map.of(), doAsync); | ||
| } | ||
|
|
There was a problem hiding this comment.
remove that, we dont want to expose that doAsync stuff to the outside. ur test can use the void count(String event, Map<String, Object> dimensions, boolean doAsync) {, tests are in the same package so package-private works for them.
There was a problem hiding this comment.
what do you mean by ??
tests are in the same package so package-private works for them.
| * Track an event execution with dimensions provided. | ||
| * | ||
| * @param event the event to save | ||
| * @param dimensions the dimensions to save |
There was a problem hiding this comment.
please dont change the javadoc, it was this:
* Track an event execution with additional contextual data.
*
* @param event the name of the event to record (e.g. "user_signup", "purchase")
* @param dimensions optional key-value pairs providing extra context about the event. These are
* often referred to as "metadata" and can include things like: userId: "12345", name:
* "John Smith", channel_name: "chit-chat" etc. This data helps with filtering, grouping,
* and analyzing events later. Note: A value for a metric should be a Java primitive
* (String, int, double, long float).
*There was a problem hiding this comment.
There was absolutely no javadoc
| /** | ||
| * | ||
| * @param event the event to save | ||
| * @param happenedAt the moment when the event is dispatched | ||
| * @param dimensionsJson optional JSON-serialized dimensions, or null | ||
| */ |
There was a problem hiding this comment.
ok and I'll add a description
firasrg
changed the title
2 participants
What
Adds the first unit test for the analytics system.
MetricsTeststo verifyMetrics#count(...)persists a metric event;Metricsexecutor service so tests can shut it down cleanly;org.togetherjava.tjbotin test runs;Why
Metricsperforms asynchronous database writes, so having direct test coverage here improves confidence in analytics persistence and makes future refactors safer.Validation
:application:test --tests org.togetherjava.tjbot.features.MetricsTests