Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
16 changes: 16 additions & 0 deletions CHANGELOG
Original file line number Diff line number Diff line change
Expand Up @@ -5,3 +5,19 @@
See the changelog report for further details:

http://m2.modularity.net.au/projects/ical4j-vcard/changelog.html

------------------------------------------------------------------------

Tests migrated from JUnit 4 to JUnit 5 Jupiter. The vintage engine bridge
has been removed; all Java tests now use org.junit.jupiter.api and
org.junit.jupiter.params exclusively.

BREAKING (test sources only): the abstract base classes
net.fortuna.ical4j.vcard.PropertyTest
net.fortuna.ical4j.vcard.ParameterTest
no longer have a constructor or fields. Subclasses must now provide
`public static Stream<Arguments> parameters()` instead of the previous
JUnit 4 `@Parameters` Collection<Object[]> + constructor injection.
Test classes are not published in the main jar so external impact is
unlikely; downstream consumers depending on these test bases need the
same migration.
7 changes: 3 additions & 4 deletions gradle/libs.versions.toml
Original file line number Diff line number Diff line change
@@ -1,11 +1,10 @@
[versions]
ical4j = "4.2.1"
ical4j = "4.2.4"
commons-io = "2.21.0"
groovy = "3.0.25"
log4j = "2.25.2"
spock = "2.4-M7-groovy-3.0"
junitVintage = "5.14.1"

junit = "5.14.1"

[libraries]
ical4j = { module = "org.mnode.ical4j:ical4j", version.ref = "ical4j" }
Expand All @@ -20,5 +19,5 @@ log4j-slf4j2 = { module = "org.apache.logging.log4j:log4j-slf4j2-impl", version.
spock-bom = { module = "org.spockframework:spock-bom", version.ref = "spock"}
spock-core = { module = "org.spockframework:spock-core", version.ref = "spock"}

junit-jupiter = { group = "org.junit.jupiter", name="junit-jupiter", version.ref = "junitVintage"}
junit-jupiter = { group = "org.junit.jupiter", name="junit-jupiter", version.ref = "junit"}
junit-platform = { module = "org.junit.platform:junit-platform-launcher" }
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
schema: spec-driven
created: 2026-05-24
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
## Context

The Gradle test task uses `useJUnitPlatform()`. The test runtime classpath currently contains:
- `org.junit.jupiter:junit-jupiter:5.14.1` (declared in `libs.versions.toml` under the misleading version key `junitVintage`)
- `org.junit.platform:junit-platform-launcher` (declared as `testRuntimeOnly`)
- `junit:junit:4.13.2` (transitive via `org.codehaus.groovy:groovy-test`, which is why the JUnit 4 tests compile)
- `org.spockframework:spock-core` (runs Groovy specs)

What is **not** on the classpath: `org.junit.vintage:junit-vintage-engine`. The JUnit Platform launcher only discovers tests via registered engines, so every class annotated with `org.junit.Test` (~67 files) is silently skipped at the discovery phase. The Groovy/Spock specs are unaffected and stay healthy throughout this change.

Two parameterized base classes carry the bulk of the property/parameter coverage:
- `net.fortuna.ical4j.vcard.PropertyTest` — extended by 41 subclasses under `property/`.
- `net.fortuna.ical4j.vcard.ParameterTest` — extended by 6 subclasses under `parameter/`.

In the JUnit 4 model, each subclass overrides a `static Collection<Object[]> parameters()` annotated with `@Parameters`, and the inherited `@Test` methods consume the parameter set via constructor injection. JUnit 5 has no class-level parameterized runner, so this inheritance shape needs to change.

## Goals / Non-Goals

**Goals:**
- All Java test classes use only `org.junit.jupiter.api` and `org.junit.jupiter.params` APIs.
- Tests previously hidden by missing vintage engine are visible in CI reports as either passing, failing, or explicitly disabled.
- A single test framework remains in active use (Jupiter); vintage engine is removed once unused.
- The parameterized base-class pattern is preserved but simplified — base classes hold the test methods, subclasses provide data only.
- The misleading `junitVintage` version key is renamed to `junit`.

**Non-Goals:**
- Migrating or restructuring Groovy/Spock specs.
- Adding new test coverage. Tests that surface as failing under the vintage bridge are triaged (fix the test or the underlying code) but no new scenarios are written as part of this change.
- Bumping the JUnit Jupiter or other framework versions.
- Removing the transitive `junit:junit:4.13.2` brought in by Groovy. It is harmless once nothing imports it.

## Decisions

### Decision 1: Bridge with vintage engine before migrating

**Decision:** Add `org.junit.vintage:junit-vintage-engine` as `testRuntimeOnly` first, triage the failures it surfaces, then migrate file-by-file, then drop vintage.

**Rationale:** The current state is undetected silent skips. The fastest path to "we know what's actually broken" is registering the engine that runs those tests. Skipping this step means migrating ~67 files blind — every Jupiter rewrite would simultaneously be the first execution of that test in months, mixing two failure modes (migration bugs vs. pre-existing failures).

**Alternatives considered:**
- *Big-bang rewrite without vintage*: One large PR rewrites all tests. Rejected — large diff, high review cost, conflates "broken before" with "broken by migration."
- *Permanently keep vintage*: Smallest patch. Rejected — leaves the codebase with two test frameworks indefinitely and contradicts the stated goal of a single modern stack.

### Decision 2: Reshape the parameterized base classes

**Decision:** `PropertyTest` and `ParameterTest` become abstract classes containing `@ParameterizedTest` + `@MethodSource("parameters")` methods that accept arguments directly. Each subclass overrides `static Stream<Arguments> parameters()` to supply data. No constructor state, no `@RunWith`.

**Rationale:** Jupiter resolves `@MethodSource` by name at the test class being executed, including methods inherited from `static` providers defined in subclasses. This keeps the "one place to add a property test, one method to supply data" ergonomics of the current design while removing the JUnit 4 runner machinery and the awkward constructor-stored fields.

**Alternatives considered:**
- *Flatten each subclass*: Inline test methods into every subclass. Rejected — would multiply ~7 methods × 47 subclasses (~330 method copies). The base-class pattern earned its keep here.
- *`@TestFactory` returning `DynamicTest`s*: Possible but obscures the parameter set in stack traces and Gradle's test reports. Rejected for ergonomics.

### Decision 3: Phase boundaries are PR boundaries

**Decision:** Each phase lands as its own PR.
- **Phase 1 PR:** Add vintage engine; rename version key; triage and resolve any test failures (fix or `@Disabled` with linked issue). CI green at end.
- **Phase 2 PRs:** Migrate tests in batches. Suggested batch boundaries: (a) `parameter/` directory (6 files + `ParameterTest` base), (b) the 21 root `vcard/` non-base tests, (c) `property/` directory in 2-3 sub-batches grouped by similarity (41 files + `PropertyTest` base). Each batch keeps the vintage engine in place so unmigrated tests still run.
- **Phase 3 PR:** Remove `junit-vintage-engine` from `build.gradle` and `libs.versions.toml`. Verify no `org.junit.Test`/`org.junit.Assert`/`org.junit.runner.*` imports remain.

**Rationale:** Reviewable diffs, CI stays green throughout, easy to bisect if a regression appears.

### Decision 4: Rename `junitVintage` to `junit` now (phase 1)

**Decision:** Rename the version reference in `libs.versions.toml` during phase 1, even though it's cosmetic.

**Rationale:** Phase 1 is the natural moment — we're touching the JUnit section, and the misleading name (vintage points at jupiter) actively misleads anyone reading the build file. Leaving it would also create confusion when we *actually* add a vintage entry alongside it.

## Risks / Trade-offs

- **[Risk] Phase 1 surfaces many failing tests at once** → Time-box triage. For genuinely broken-by-drift tests with no quick fix, mark `@Ignore` (JUnit 4) during phase 1 with a TODO and a linked issue; convert to `@Disabled` in phase 2 when the file is migrated. Don't let triage block the bridge from landing.
- **[Risk] The `PropertyTest`/`ParameterTest` `@MethodSource` reshape regresses parameter coverage** → Migrate the base + one subclass first as a pilot; verify report XML shows the same count of test method × parameter row combinations as before (under vintage). Only then proceed with the bulk.
- **[Risk] Long-lived branch drifts if phase 2 is spread over many PRs** → Each phase 2 PR is independently mergeable because vintage stays in place. Rebase friction is minimal — the test files don't overlap with active feature work.
- **[Risk] External consumers depend on `PropertyTest`/`ParameterTest` as a test base** → Low probability (test classes aren't published in the main jar), but worth a note in the release/CHANGELOG entry. If anyone has been (re)using them, they get the same migration recipe.
- **[Trade-off] Keeping the transitive `junit:junit:4.13.2` from Groovy** → Acceptable. It costs nothing at runtime and removing it would mean restructuring Groovy test dependencies for no real gain.

## Migration Plan

1. **Phase 1 (this change, PR #1):** vintage bridge + version-key rename + triage. CI green with all previously-hidden tests now running.
2. **Phase 2 (subsequent PRs):** mechanical Jupiter migration in batches. Vintage stays.
3. **Phase 3 (final PR):** drop vintage; verify no `org.junit.*` (non-jupiter) imports remain via grep gate.

Rollback: each phase is independently revertable. Phase 1 revert restores the silent-skip state (no harm); phase 2 reverts restore vintage execution of the affected files; phase 3 revert puts vintage back.
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
## Why

The project's Java test suite is written in JUnit 4 style (`@RunWith(Parameterized.class)`, `org.junit.Test`, `org.junit.Assert`), but the Gradle build runs with `useJUnitPlatform()` and only the JUnit Jupiter engine on the test runtime classpath. There is no `junit-vintage-engine`, so the JUnit Platform launcher silently skips every JUnit 4 test class at discovery time. Build reports under `build/test-results/test/` confirm only Spock specs and the handful of true Jupiter tests are executed; ~67 JUnit 4 test classes — including the parameterized `PropertyTest`/`ParameterTest` hierarchies covering most vCard property and parameter behavior — are not running at all. This silently masks regressions. The end state is a single, modern test framework (JUnit 5 Jupiter) with no vintage shim, but we will get there in phases so CI is green before the bulk migration begins.

## What Changes

- Add `junit-vintage-engine` to the test runtime classpath as a temporary bridge so the existing JUnit 4 tests are discovered and executed under JUnit Platform. Fix any failures that surface.
- Rename the misleading version key `junitVintage` in `gradle/libs.versions.toml` to `junit` (it points at `junit-jupiter` and was never wired to vintage).
- Migrate all ~67 JUnit 4 test classes to JUnit 5 Jupiter:
- Replace `@RunWith(Parameterized.class)` + `@Parameters` with `@ParameterizedTest` + `@MethodSource`.
- Replace `org.junit.Test`/`Before`/`After`/`Assert` with the `org.junit.jupiter.api` equivalents.
- Restructure the `PropertyTest` / `ParameterTest` parameterized base-class pattern: the base class provides `@ParameterizedTest` methods that take arguments directly; the 41 `PropertyTest` subclasses and 6 `ParameterTest` subclasses become `@MethodSource` data providers (no constructor state).
- Remove `junit-vintage-engine` once no JUnit 4 imports remain. **BREAKING** for downstream test inheritance: any external consumer extending `PropertyTest`/`ParameterTest` would need to follow the same migration. (The test classes are not published, so external impact is unlikely.)

## Capabilities

### New Capabilities
- `test-framework`: Defines the test framework, engines on the JUnit Platform classpath, and the conventions (Jupiter APIs, parameterized test pattern, base-class shape) that test code in this repository must follow.

### Modified Capabilities
<!-- None. No published-behavior specs exist; this change reshapes test infrastructure, captured by the new `test-framework` capability. -->

## Impact

- **Code**: `src/test/java/**` — ~67 Java test classes rewritten. Groovy/Spock specs under `src/test/groovy/**` are unaffected (already run via spock-core on JUnit Platform).
- **Build**: `gradle/libs.versions.toml` (add vintage engine entry, rename key); `build.gradle` (add `testRuntimeOnly libs.junit.vintage` during phase 1, remove in phase 3).
- **CI**: Phase 1 will likely surface previously-hidden test failures. Each must be triaged (fix vs. mark `@Disabled` with an issue link) before phase 2 begins.
- **Dependencies**: Temporary addition of `org.junit.vintage:junit-vintage-engine`. The transitive `junit:junit:4.13.2` from `groovy-test` continues to satisfy compile-time JUnit 4 imports during phase 2 and disappears as a usage once phase 2 completes (the artifact itself remains on the classpath via Groovy, which is fine).
- **Risk**: Low. Vintage is the JUnit team's supported bridge; migration is mechanical per file. The base-class restructure is the only design choice and is contained to two files plus their subclasses.
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
## ADDED Requirements

### Requirement: JUnit Platform discovers all Java tests
The build SHALL ensure every Java test class under `src/test/java/` is discovered and executed by the configured JUnit Platform engines. No Java test class shall be silently skipped due to a missing engine on the test runtime classpath.

#### Scenario: Every Java test class appears in build reports
- **WHEN** the Gradle `test` task runs to completion
- **THEN** `build/test-results/test/` contains a `TEST-*.xml` report for every non-abstract Java test class under `src/test/java/`
- **AND** no test class is present in source but absent from the report directory

#### Scenario: A new test framework annotation requires a registered engine
- **WHEN** a test class is added that uses a framework whose engine is not on the test runtime classpath
- **THEN** the build either registers the appropriate engine or rejects the test class
- **AND** the situation where the class compiles but is never executed must not occur

### Requirement: Java tests use JUnit 5 Jupiter APIs
All Java test classes under `src/test/java/` SHALL use only `org.junit.jupiter.api.*` and `org.junit.jupiter.params.*` for test annotations, lifecycle hooks, assertions, and parameterized data. Imports from `org.junit.Test`, `org.junit.Assert`, `org.junit.Before`, `org.junit.After`, `org.junit.runner.*`, or `org.junit.runners.*` shall not appear in any test source file once the migration is complete.

#### Scenario: No JUnit 4 imports remain after migration
- **WHEN** the migration completes (end of phase 3)
- **AND** a grep is run for `org.junit.Test`, `org.junit.Assert`, `org.junit.Before`, `org.junit.After`, `org.junit.runner`, or `org.junit.runners` under `src/test/java/`
- **THEN** zero matches are returned

#### Scenario: Single test method
- **WHEN** a test class has a single test method
- **THEN** the method is annotated with `org.junit.jupiter.api.Test`
- **AND** assertions use `org.junit.jupiter.api.Assertions.*`

#### Scenario: Setup and teardown
- **WHEN** a test class needs per-test setup or teardown
- **THEN** lifecycle methods are annotated with `@BeforeEach` / `@AfterEach`
- **AND** per-class lifecycle uses `@BeforeAll` / `@AfterAll` on `static` methods

### Requirement: Parameterized tests use Jupiter `@ParameterizedTest` + `@MethodSource`
Parameterized test classes SHALL use `@ParameterizedTest` together with `@MethodSource` referencing a `static` method that returns `Stream<Arguments>` (or equivalent). Subclasses that contribute only data SHALL override the source method; the inherited test method definitions live in the abstract base class.

#### Scenario: Base class defines parameterized test methods
- **WHEN** a parameterized base class such as `PropertyTest` or `ParameterTest` defines a test method
- **THEN** the method is annotated with `@ParameterizedTest` and `@MethodSource("parameters")`
- **AND** test data flows in as method arguments
- **AND** the class is `abstract` and holds no instance fields populated from constructor parameters

#### Scenario: Subclass supplies parameter data
- **WHEN** a subclass extends a parameterized base class to test a specific property or parameter
- **THEN** the subclass defines `static Stream<Arguments> parameters()` returning the test rows
- **AND** the subclass does not declare a constructor that accepts test parameters
- **AND** the subclass inherits all `@ParameterizedTest` methods from the base without redefinition

### Requirement: Test framework dependencies are declared explicitly
The `gradle/libs.versions.toml` catalog SHALL declare JUnit dependencies with accurate, non-misleading names. Version reference keys SHALL match the artifact they resolve.

#### Scenario: Version reference key matches the artifact it resolves
- **WHEN** a version reference is declared for a JUnit dependency
- **THEN** the key name reflects the artifact it points at (e.g., a key named `junit` resolves a JUnit artifact; a key named `junit-vintage` resolves the vintage engine)
- **AND** no key resolves an artifact whose name contradicts the key

#### Scenario: Vintage engine is only present while needed
- **WHEN** any Java test class still uses JUnit 4 APIs
- **THEN** `org.junit.vintage:junit-vintage-engine` is declared as `testRuntimeOnly`
- **WHEN** zero Java test classes use JUnit 4 APIs
- **THEN** the vintage engine declaration is removed from the build
Loading
Loading