Skip to content

Unit Tests for Metrics #1439

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.

Sign up for GitHub

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

Open
firasrg wants to merge 4 commits into
base: develop
Choose a base branch
from
Open

Unit Tests for Metrics #1439

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c75dc22
Adding a first unit test for Metrics with logging
firasrg Mar 13, 2026
7ba857d
Merge remote-tracking branch 'origin/develop' into firasrg/setting-an…
firasrg Apr 25, 2026
29bc42e
Updates metrics test: using doAsync flag to check event persistence o…
firasrg Apr 27, 2026
7b7a676
Metrics: revert access modifier for processEvent from package-private…
firasrg Apr 27, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 39 additions & 10 deletions application/src/main/java/org/togetherjava/tjbot/features/analytics/Metrics.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,26 @@ public void count(String event) {
count(event, Map.of());
}

/**
* Track an event execution with dimensions provided.
*
* @param event the event to save
* @param dimensions the dimensions to save
Comment on lines +48 to +51
Copy link
Copy Markdown
Member

@Zabuzard Zabuzard Apr 28, 2026

Choose a reason for hiding this comment

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

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).
     *

Copy link
Copy Markdown
Contributor Author

@firasrg firasrg Apr 28, 2026

Choose a reason for hiding this comment

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

There was absolutely no javadoc

Copy link
Copy Markdown
Member

@Zabuzard Zabuzard Apr 29, 2026

Choose a reason for hiding this comment

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

There was. The method that was previously public facing is now not public facing anymore and the method you made public facing has a different, less detailled, javadoc:
grafik

*/
public void count(String event, Map<String, Object> dimensions) {
count(event, dimensions, true);
}

/**
* 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);
}

Comment on lines +57 to +66
Copy link
Copy Markdown
Member

@Zabuzard Zabuzard Apr 28, 2026

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

@firasrg firasrg Apr 28, 2026

Choose a reason for hiding this comment

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

what do you mean by ??

tests are in the same package so package-private works for them.

Copy link
Copy Markdown
Member

@Zabuzard Zabuzard Apr 29, 2026

Choose a reason for hiding this comment

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

(talked about it in discord)

/**
* Track an event execution with additional contextual data.
*
Expand All @@ -54,14 +74,19 @@ public void count(String event) {
* and analyzing events later. Note: A value for a metric should be a Java primitive
* (String, int, double, long float).
*/
public void count(String event, Map<String, Object> dimensions) {
void count(String event, Map<String, Object> dimensions, boolean doAsync) {
logger.debug("Counting new record for event: {}", event);

Instant happenedAt = Instant.now();
String serializedDimensions = serializeDimensions(dimensions);
String serializedDimensions = dimensions.isEmpty() ? null : serializeDimensions(dimensions);

Runnable task = () -> processEvent(event, happenedAt, serializedDimensions);

service.submit(() -> processEvent(event, happenedAt,
dimensions.isEmpty() ? null : serializedDimensions));
if (doAsync) {
service.submit(task);
} else {
task.run();
}
}

private static String serializeDimensions(Map<String, Object> dimensions) {
Expand All @@ -72,12 +97,6 @@ private static String serializeDimensions(Map<String, Object> dimensions) {
}
}

/**
*
* @param event the event to save
* @param happenedAt the moment when the event is dispatched
* @param dimensionsJson optional JSON-serialized dimensions, or null
*/
Comment on lines -75 to -80
Copy link
Copy Markdown
Member

@Zabuzard Zabuzard Apr 28, 2026

Choose a reason for hiding this comment

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

dont remove that

Copy link
Copy Markdown
Contributor Author

@firasrg firasrg Apr 28, 2026

Choose a reason for hiding this comment

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

ok and I'll add a description

private void processEvent(String event, Instant happenedAt, @Nullable String dimensionsJson) {
database.write(context -> context.newRecord(MetricEvents.METRIC_EVENTS)
.setEvent(event)
Expand All @@ -86,4 +105,14 @@ private void processEvent(String event, Instant happenedAt, @Nullable String dim
.insert());
}

/**
* Exposes the underlying executor service.
* <p>
* Intended for test teardown only.
*
* @return the executor service backing this instance
*/
public ExecutorService getExecutorService() {
return service;
}
Comment on lines +115 to +117
Copy link
Copy Markdown
Member

@Zabuzard Zabuzard Mar 13, 2026

Choose a reason for hiding this comment

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

undo, this won't be needed (see other comment why)

Copy link
Copy Markdown
Member

@Zabuzard Zabuzard Apr 28, 2026

Choose a reason for hiding this comment

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

(not done yet)

}
47 changes: 47 additions & 0 deletions application/src/test/java/org/togetherjava/tjbot/features/MetricsTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
package org.togetherjava.tjbot.features;

import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;

import org.togetherjava.tjbot.db.Database;
import org.togetherjava.tjbot.db.generated.tables.MetricEvents;
import org.togetherjava.tjbot.db.generated.tables.records.MetricEventsRecord;
import org.togetherjava.tjbot.features.analytics.Metrics;

import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertNull;

final class MetricsTests {
Copy link
Copy Markdown
Member

@Zabuzard Zabuzard Mar 13, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

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.

  1. create a package private method in Metrics called countBlocking. its identical to count but it won't use the executor service. so it creates the instant and then calls processEvent directly
  2. in ur unit test this will be the method ur testing against now. without the async stuff the test will simplify a lot
  3. 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)

Copy link
Copy Markdown
Contributor Author

@firasrg firasrg Apr 25, 2026

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member

@Zabuzard Zabuzard Apr 27, 2026

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member

@Zabuzard Zabuzard Apr 28, 2026

Choose a reason for hiding this comment

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

(not done yet)

Copy link
Copy Markdown
Contributor Author

@firasrg firasrg Apr 28, 2026

Choose a reason for hiding this comment

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

Why do we have to create an assertion method assertCloseEnough ?

Copy link
Copy Markdown
Member

@Zabuzard Zabuzard Apr 29, 2026

Choose a reason for hiding this comment

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

(talked about it in discord)

private Database database;
private Metrics metrics;

@BeforeEach
void setUp() {
database = Database.createMemoryDatabase(MetricEvents.METRIC_EVENTS);
metrics = new Metrics(database);
}

@AfterEach
void tearDown() {
metrics.getExecutorService().shutdownNow();
}

@Test
void countWithDoAsyncFalsePersists() {

String testEvent = "metrics_test_event";

metrics.count(testEvent, false);

MetricEventsRecord savedRecord =
database.read(context -> context.selectFrom(MetricEvents.METRIC_EVENTS).fetchOne());

assertNotNull(savedRecord);

assertEquals(testEvent, savedRecord.get(MetricEvents.METRIC_EVENTS.EVENT));
assertNull(savedRecord.get(MetricEvents.METRIC_EVENTS.DIMENSIONS));
}

}
Loading