From f23a136760c9c94c96748b22c3d10c8bb9687d7d Mon Sep 17 00:00:00 2001 From: Sam Brannen Date: Wed, 19 Dec 2018 14:18:03 +0100 Subject: [PATCH] Avoid NullPointerException when creating Executions from filtered events Prior to this commit, the logic in Executions.createExecutions() could result in a NullPointerException if the executions were being created from filtered events. For example, an attempt to create executions from `testEvents.failed().executions()` resulted in a NullPointerException since the "started" event (needed to determine the start Instant) was no longer present in the event list. This commit fixes this bug by avoiding the creation of Execution instances if the "started" event is absent. Consequently, an invocation such as `testEvents.failed().executions().count()` now returns `0` instead of throwing a NullPointerException. Issue: #1356 --- .../platform/testkit/engine/Executions.java | 30 +++-- .../engine/ExecutionsIntegrationTests.java | 103 ++++++++++++++++++ 2 files changed, 124 insertions(+), 9 deletions(-) create mode 100644 platform-tests/src/test/java/org/junit/platform/testkit/engine/ExecutionsIntegrationTests.java diff --git a/junit-platform-testkit/src/main/java/org/junit/platform/testkit/engine/Executions.java b/junit-platform-testkit/src/main/java/org/junit/platform/testkit/engine/Executions.java index 5d3a10ebc7d1..18794de2d35d 100644 --- a/junit-platform-testkit/src/main/java/org/junit/platform/testkit/engine/Executions.java +++ b/junit-platform-testkit/src/main/java/org/junit/platform/testkit/engine/Executions.java @@ -261,20 +261,32 @@ private static List createExecutions(List events) { break; } case SKIPPED: { - Instant startInstant = executionStarts.get(event.getTestDescriptor()); - Execution skippedEvent = Execution.skipped(event.getTestDescriptor(), - startInstant != null ? startInstant : event.getTimestamp(), event.getTimestamp(), + // Based on the Javadoc for EngineExecutionListener.executionSkipped(...), + // a skipped descriptor must never be reported as started or finished, + // but just in case a TestEngine does not adhere to that contract, we + // make an attempt to remove any tracked "started" event. + executionStarts.remove(event.getTestDescriptor()); + + // Use the same timestamp for both the start and end Instant values. + Instant timestamp = event.getTimestamp(); + + Execution skippedEvent = Execution.skipped(event.getTestDescriptor(), timestamp, timestamp, event.getRequiredPayload(String.class)); executions.add(skippedEvent); - executionStarts.remove(event.getTestDescriptor()); break; } case FINISHED: { - Execution finishedEvent = Execution.finished(event.getTestDescriptor(), - executionStarts.get(event.getTestDescriptor()), event.getTimestamp(), - event.getRequiredPayload(TestExecutionResult.class)); - executions.add(finishedEvent); - executionStarts.remove(event.getTestDescriptor()); + Instant startInstant = executionStarts.remove(event.getTestDescriptor()); + Instant endInstant = event.getTimestamp(); + + // If "started" events have already been filtered out, it is no + // longer possible to create a legitimate Execution for the current + // descriptor. So we effectively ignore it in that case. + if (startInstant != null) { + Execution finishedEvent = Execution.finished(event.getTestDescriptor(), startInstant, + endInstant, event.getRequiredPayload(TestExecutionResult.class)); + executions.add(finishedEvent); + } break; } default: { diff --git a/platform-tests/src/test/java/org/junit/platform/testkit/engine/ExecutionsIntegrationTests.java b/platform-tests/src/test/java/org/junit/platform/testkit/engine/ExecutionsIntegrationTests.java new file mode 100644 index 000000000000..af06cc95f8f6 --- /dev/null +++ b/platform-tests/src/test/java/org/junit/platform/testkit/engine/ExecutionsIntegrationTests.java @@ -0,0 +1,103 @@ +/* + * Copyright 2015-2018 the original author or authors. + * + * All rights reserved. This program and the accompanying materials are + * made available under the terms of the Eclipse Public License v2.0 which + * accompanies this distribution and is available at + * + * http://www.eclipse.org/legal/epl-v20.html + */ + +package org.junit.platform.testkit.engine; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.fail; +import static org.junit.jupiter.api.Assumptions.assumeTrue; +import static org.junit.platform.engine.discovery.DiscoverySelectors.selectClass; + +import org.junit.jupiter.api.Disabled; +import org.junit.jupiter.api.Test; + +/** + * Integration tests for {@link Executions}. + * + * @since 1.4 + */ +class ExecutionsIntegrationTests { + + @Test + void executionsFromSkippedTestEvents() { + Events testEvents = getTestEvents(); + + // We expect 1 for both of the following cases, since an Execution can + // be created for a "skipped event even if "started" and "finished" events + // are filtered out. + assertThat(testEvents.executions().skipped().count()).isEqualTo(1); + assertThat(testEvents.skipped().executions().count()).isEqualTo(1); + } + + @Test + void executionsFromSucceededTestEvents() { + Events testEvents = getTestEvents(); + + // We expect 1 if the executions are created BEFORE filtering out "finished" events. + assertThat(testEvents.executions().succeeded().count()).isEqualTo(1); + // We expect 0 if the executions are created AFTER filtering out "finished" events. + assertThat(testEvents.succeeded().executions().count()).isEqualTo(0); + } + + @Test + void executionsFromAbortedTestEvents() { + Events testEvents = getTestEvents(); + + // We expect 1 if the executions are created BEFORE filtering out "started" events. + assertThat(testEvents.executions().aborted().count()).isEqualTo(1); + // We expect 0 if the executions are created AFTER filtering out "started" events. + assertThat(testEvents.aborted().executions().count()).isEqualTo(0); + } + + @Test + void executionsFromFailedTestEvents() { + Events testEvents = getTestEvents(); + + // We expect 1 if the executions are created BEFORE filtering out "started" events. + assertThat(testEvents.executions().failed().count()).isEqualTo(1); + // We expect 0 if the executions are created AFTER filtering out "started" events. + assertThat(testEvents.failed().executions().count()).isEqualTo(0); + } + + private Events getTestEvents() { + Events testEvents = EngineTestKit.engine("junit-jupiter")// + .selectors(selectClass(ExampleTestCase.class))// + .execute()// + .tests(); + + testEvents.assertStatistics(stats -> stats.skipped(1).started(3).succeeded(1).aborted(1).failed(1)); + + return testEvents; + } + + static class ExampleTestCase { + + @Test + @Disabled + void skippedTest() { + } + + @Test + void succeedingTest() { + } + + @Test + void abortedTest() { + assumeTrue(false); + } + + @Test + void failingTest() { + fail("Boom!"); + } + + } + +}