Migrate testng-core's internal tests from org.testng.Assert to AssertJ#3278
Migrate testng-core's internal tests from org.testng.Assert to AssertJ#3278juherr wants to merge 4 commits into
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
b180e41 to
32ce21f
Compare
32ce21f to
1ac1eb7
Compare
e7e85d7 to
31f3139
Compare
31f3139 to
6a0c87e
Compare
861c3e6 to
dd73dd9
Compare
1d33167 to
dcebefa
Compare
|
@krmahadevan Sorry for the huge diff. I used openrewrite and claude to make it. |
@juherr - No issues. I will go through this asap. |
|
@krmahadevan Before we wrap this up, I'd like your input on how we handle the assertion issues and the overall sequencing, because it reframes this PR. There are ~10 open tickets about Rather than closing them all as If we go that way, I'd rather split this work into two phases instead of doing everything here: Phase 1 — extract the module (this PR, trimmed down):
Phase 2 — AssertJ implementation, in the new repo:
This keeps core's change minimal and reversible, and avoids baking an AssertJ dependency / behaviour change into core right before the module leaves anyway. WDYT? Mainly want to align on: (a) hand-off vs. removal, (b) dropping |
|
@juherr - I think your proposal of 2 phased approach makes a lot more sense. Although when compared to |
|
@krmahadevan Thanks, that makes sense. To keep things moving faster, I would actually prefer starting directly with the dedicated My proposed sequence would be:
That would avoid doing the release setup twice: once in the main repository, then again in the dedicated repository. But I am fine either way. I just wanted to propose this approach because I think it may be faster and cleaner from an ownership/release perspective. I'll let you decide which path you prefer:
|
|
@juherr - That sounds good. Lets start with Create a dedicated testng-asserts repository under testng-team. and then work backwards to replace what exists within TestNG with this dependency. |
6840777 to
86ae44e
Compare
testng-core's own tests are migrated off org.testng.Assert to AssertJ using the OpenRewrite recipe TestNgToAssertj (rewrite-testing-frameworks 3.39.0), run as a one-shot -- the temporary plugin config is not retained. - testng-core/src/test: ~500 files rewritten to org.assertj.core.api.Assertions - testng.testing convention: bump assertj-core 3.23.1 -> 3.27.7 (the recipe emits the no-arg Assertions.fail() added in AssertJ 3.26.0) - ParameterConstructorTest: fix float offset (Offset.offset(0.1f)) - BasicSample.kt: kept on org.testng.Assert (OpenRewrite Kotlin support is limited; the migrated AssertJ no-arg fail() breaks Kotlin type inference) A few testng-core tests exercising org.testng.Assert behaviour directly (DependentTest, AssertionsTestSample.groovy) intentionally keep using it.
The TestNgToAssertj recipe only ports the API: it turns Assert.assertX(cond) into assertThat(cond).isTrue(), so predicate expressions kept the original boolean shape and produced unhelpful "expected true but was false" failures. Run the AssertJ best-practices cleanup (the org.openrewrite.java.testing.assertj.Assertj aggregate, minus the cross-framework migrators so the intentional JUnit-interop fixtures are left untouched) as a one-shot over testng-core's tests. This rewrites predicates to dedicated assertions with proper failure messages, e.g.: assertThat(list.size()).isEqualTo(3) -> assertThat(list).hasSize(3) assertThat(time < 2000).isTrue() -> assertThat(time).isLessThan(2000) assertThat(c.isEmpty()).isFalse() -> assertThat(c).isNotEmpty() assertThat(s.contains(x)).isTrue() -> assertThat(s).contains(x) assertThat(n).isEqualTo(0) -> assertThat(n).isZero() instanceof predicates are not handled by any existing recipe (tracked upstream as openrewrite/rewrite-testing-frameworks#1030), so they were converted by hand: assertThat(o instanceof T).isTrue() -> assertThat(o).isInstanceOf(T.class) A few assertions the recipes can't reach were also tidied by hand (e.g. in GraphTest): reversed actual/expected and boolean-OR equality chains: assertThat(n).isEqualTo(x.size()) -> assertThat(x).hasSize(n) assertThat(a.equals(x) || b.equals(x)) -> assertThat(x).isIn(a, b) .isTrue() One assertion was fixed by hand too: the recipe rewrote `null != x` to the ambiguous `assertThat(null).isNotSameAs(x)` (tracked as PicnicSupermarket/error-prone-support#2276); replaced with `assertThat(x).isNotNull()`.
Idiomatic simplifications surfaced while validating the rewrite-testing-frameworks 3.40.0 snapshot against the suite, which the earlier hand-finishing pass had not covered: - array length: assertThat(arr.length).isEqualTo(n) -> assertThat(arr).hasSize(n), assertThat(arr.length).isZero() -> assertThat(arr).isEmpty() - assertThat(a.length).isEqualTo(b.size()) -> assertThat(a).hasSameSizeAs(b) - assertThat(x).isNotEqualTo(0) / isEqualTo(0) -> isNotZero() / isZero() Behaviour-preserving; full testng-core suite green (12474 passed, 0 failed).
86ae44e to
54300a2
Compare
With testng-core's own tests fully on AssertJ, the last org.testng.Assert /
org.testng.asserts usages are migrated and the testng-asserts dependency is
dropped from both modules:
- DependentTest: Assert.assertEquals -> assertThat(...).isEqualTo(...)
- issue2400/IssueTest, test/tmp/A: org.testng.asserts.SoftAssert ->
org.assertj.core.api.SoftAssertions
- BasicSample.kt: org.testng.Assert.fail -> AssertJ fail<Nothing>("...")
- AssertionsTestSample.groovy (issue2854): org.testng.Assert.assertEquals ->
AssertJ assertThat (the org.testng.Assert Groovy-overload coverage now lives
in the standalone testng-asserts repo, GITHUB-3034)
- testng-core: drop testImplementation(projects.testngAsserts)
- testng-api: drop api(projects.testngAsserts)
org.testng.AssertJUnit (JUnit-style assertions used by the internal
ComparisonCriteria) stays in testng-core and is unaffected. testng-asserts is
still built and embedded in the org.testng:testng fat jar; that and the BOM
retarget the standalone org.testng:testng-asserts once it is published.
#20) * Delegate behaviour-identical assertions to AssertJ Port the AssertJ-backed assertion implementation from testng-team/testng#3278 into this standalone artifact, so it can be dropped from the parent PR. - Assert: delegate assertNull/assertNotNull/assertSame/assertNotSame and assertThrows to AssertJ (behaviour-identical, only the failure message differs); remove the now-unused failSame/failNotSame helpers. Equality comparison intentionally keeps TestNG semantics (documented NOTE). - Mark org.testng.Assert @deprecated (recommend AssertJ); add @SuppressWarnings("deprecation") on Assertion. - pom: move assertj-core to compile scope (the main code depends on it). - docs: add MIGRATING_ASSERTIONS.md and link it from the README. expectThrows is kept unchanged (it returns the caught exception, which AssertJ's fluent API does not expose). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Delegate assertTrue/assertFalse/fail to AssertJ Follow-up to the AssertJ delegation: extend it to the remaining behaviour-identical entry points. - assertTrue/assertFalse -> assertThat(condition).as(message).isTrue()/isFalse() - fail(message) and fail(message, cause) -> org.assertj.core.api.Assertions.fail(...) These flow through the OO Assertion/SoftAssert wrappers automatically (they delegate to org.testng.Assert), so soft assertions now use AssertJ for these checks too. Only the failure-message text changes; the pass/fail behaviour is unchanged. Updated the message-format-sensitive tests accordingly: - assertTrue/assertFalse message regexps now match AssertJ's output. - SoftAssertTest#testAssertAllCount no longer counts report lines (AssertJ messages are multi-line); it asserts the failed message is reported once.
Summary
This PR moves TestNG away from maintaining its own assertion library, as discussed in testng-team/testng-asserts#15 and
#3197. It deprecates
org.testng.Assert, migrates TestNG's own tests to AssertJ, starts delegatingAssertto AssertJ where it is safe to do so, and turnstestng-assertsinto a separate, optionalpublished artifact.
The work is split into self-contained commits so it can be reviewed phase by phase.
Motivation
testng-assertsmodule optional.The OpenRewrite recipe
org.openrewrite.java.testing.testng.TestNgToAssertj(rewrite-testing-frameworks ≥ 3.39.0) now covers the whole
Assertclass, so users can migrateautomatically. That makes deprecation realistic.
What's in this PR
org.testng.Assert@Deprecated(since = "7.13.0", forRemoval = true)+ a migration guide (docs/MIGRATING_ASSERTIONS.md).testng-coretest files rewritten offorg.testng.Assertto AssertJ via a one-shot OpenRewrite run (no plugin left in the build);assertj-corebumped 3.23.1 → 3.27.7.assertNull/assertNotNull/assertSame/assertNotSamenow delegate to AssertJ.assertThrowsassertThrowsnow delegates to AssertJ;expectThrowsis kept (it returns the caught exception).testng-assertsstandalonetestngfat jar; published asorg.testng:testng-assertsdepending only on AssertJ.testng-assertsships as a plain jar for now (see below).Design decisions
delegated (
assertNull/NotNull/Same/NotSame,assertThrows).assertEquals/assertNotEqualsand the collection/map/array variants keep TestNG's own comparison semantics, which differ from
AssertJ on purpose (symmetric equals,
equals()not trusted againstnull, nested arrays comparedby reference unless using
assertEqualsDeep). This preserves the existing behaviour contract andits regression tests (assertNotEquals misses false inverse #2483, assertSame/assertNotSame broken after GITHUB-2296 #2486, assertNotEquals returns fast when argument is null, not calling equals(Object) #2490, Assert.assertEqualsNoOrder does not check elements deeply #2500,
ArrayEqualityAssertTest). The divergence isdocumented with a
NOTEcomment inAssert.messages. This is the only user-visible behaviour change and is noted in
CHANGES.txt.expectThrowsunchanged. It returns the caught exception, which AssertJ's fluent API does notexpose, so it stays on TestNG's implementation (its exact-message tests keep passing).
org.testng.Assertis no longer bundled in theorg.testng:testngjar. To keep using it, add:which brings AssertJ transitively. The recommended path is to migrate to AssertJ (see
docs/MIGRATING_ASSERTIONS.md).OSGi
testng-assertsis published as a plain jar, not an OSGi bundle:org.testng.AssertandFileAssertshare theorg.testngpackage with the core classes exported by thetestngbundle,so a
testng-assertsbundle re-exportingorg.testngwould create a split package. Proper OSGisupport needs dedicated design and is left as a follow-up.
Testing
:testng-asserts:test— 181 passed:testng-core:test— 12474 passed:testng-test-osgi:test— 4 passed./gradlew autostyleCheck— cleantestngjar no longer containsorg/testng/Assert*, and the publishedtestng-assertsPOM depends only onorg.assertj:assertj-core.Follow-ups (not in this PR)
testng-asserts(split-package handling).FileAssert, the OOAssertion/SoftAssertAPI).