Skip to content

Add initial unit test coverage for Metrics event persistence #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

Draft
firasrg wants to merge 1 commit into
base: develop
Choose a base branch
from
+124 −0
Draft

Add initial unit test coverage for Metrics event persistence #1439

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
1 change: 1 addition & 0 deletions application/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ dependencies {
implementation 'org.apache.commons:commons-text:1.15.0'
implementation 'com.apptasticsoftware:rssreader:3.12.0'

testImplementation 'org.assertj:assertj-core:3.27.7'
Copy link
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. we want to use raw junit in this project for now as it is easier for beginners and reflects more what they will see in the wild and might already be familiar with from their own projects and university etc

testImplementation 'org.mockito:mockito-core:5.23.0'
testImplementation "org.junit.jupiter:junit-jupiter-api:$junitVersion"
testImplementation "org.junit.jupiter:junit-jupiter-params:$junitVersion"
Expand Down
3 changes: 3 additions & 0 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 @@ -53,4 +53,7 @@ private void processEvent(String event, Instant happenedAt) {
.insert());
}

public ExecutorService getExecutorService() {
return service;
}
Comment on lines +56 to +58
Copy link
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)

}
118 changes: 118 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,118 @@
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.slf4j.Logger;
import org.slf4j.LoggerFactory;

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

import java.time.Duration;
import java.time.Instant;
import java.time.temporal.ChronoUnit;
import java.util.List;
import java.util.concurrent.locks.LockSupport;

import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.within;
import static org.junit.jupiter.api.Assertions.fail;

final class MetricsTests {
Copy link
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)

private static final Logger logger = LoggerFactory.getLogger(MetricsTests.class);

private static final Duration WAIT_TIMEOUT = Duration.ofSeconds(3);

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 countInsertsSingleEvent() {

final String slashPing = "slash-ping";

metrics.count(slashPing);

awaitRecords(1);

List<String> recordedEvents = readEventsOrderedById();

assertThat(recordedEvents).as("Metrics should persist the counted event in insertion order")
.containsExactly(slashPing);

assertThat(readLatestEventHappenedAt())
.as("Metrics should store a recent timestamp for event '%s' (recordedEvents=%s)",
slashPing, recordedEvents)
.isNotNull()
.isCloseTo(Instant.now(), within(5, ChronoUnit.SECONDS));
}

private void awaitRecords(int expectedAmount) {
Instant deadline = Instant.now().plus(WAIT_TIMEOUT);

while (Instant.now().isBefore(deadline)) {
if (readRecordCount() == expectedAmount) {
return;
}

LockSupport.parkNanos(Duration.ofMillis(25).toNanos());

if (Thread.interrupted()) {
int actualCount = readRecordCount();

String msg = String.format(
"Interrupted while waiting for metrics writes (expectedAmount=%d, actualCount=%d, timeout=%s, events=%s)",
expectedAmount, actualCount, WAIT_TIMEOUT, readEventsOrderedById());

logger.warn(msg);

fail(msg);
}
}

int actualCount = readRecordCount();

List<String> recordedEvents = readEventsOrderedById();

String timeoutMessage = String.format(
"Timed out waiting for metrics writes (expectedAmount=%d, actualCount=%d, timeout=%s, events=%s)",
expectedAmount, actualCount, WAIT_TIMEOUT, recordedEvents);

logger.warn(timeoutMessage);

assertThat(actualCount).as(timeoutMessage).isEqualTo(expectedAmount);
}

private int readRecordCount() {
return database.read(context -> context.fetchCount(MetricEvents.METRIC_EVENTS));
}

private List<String> readEventsOrderedById() {
return database.read(context -> context.select(MetricEvents.METRIC_EVENTS.EVENT)
.from(MetricEvents.METRIC_EVENTS)
.orderBy(MetricEvents.METRIC_EVENTS.ID.asc())
.fetch(MetricEvents.METRIC_EVENTS.EVENT));

}

private Instant readLatestEventHappenedAt() {
return database.read(context -> context.select(MetricEvents.METRIC_EVENTS.HAPPENED_AT)
.from(MetricEvents.METRIC_EVENTS)
.orderBy(MetricEvents.METRIC_EVENTS.ID.desc())
.limit(1)
.fetchOne(MetricEvents.METRIC_EVENTS.HAPPENED_AT));
}
}
2 changes: 2 additions & 0 deletions application/src/test/resources/log4j2.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@
</RollingFile>
</Appenders>
<Loggers>
<Logger name="org.togetherjava.tjbot" level="debug"/>

Comment on lines +16 to +17
Copy link
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

<Root level="info">
<AppenderRef ref="Console"/>
<AppenderRef ref="File"/>
Expand Down
Loading