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 1 commit
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
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'
Comment thread
Zabuzard marked this conversation as resolved.
Outdated
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 +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)

}
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
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 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 thread
Zabuzard marked this conversation as resolved.
Outdated
<Root level="info">
<AppenderRef ref="Console"/>
<AppenderRef ref="File"/>
Expand Down
Loading