refactor!: remove Kotlin from the codebase - #255
Draft
mcollovati wants to merge 15 commits into
Draft
mcollovati wants to merge 15 commits into
mcollovati wants to merge 15 commits into
Conversation
The classes in `com.vaadin.browserless.mocks` and `com.vaadin.browserless.quarkus.mocks` are built by the framework itself — `SpringBrowserlessTest`, `QuarkusBrowserlessTest` and the application contexts construct them as local variables, and no published signature accepts or returns one. Say so, using the same wording as `BaseBrowserlessTest`, so they stay free to change. `MockWebApplicationContext` and `SpringSecurityRequestCustomizer` already carried a shorter note; they now use the full sentence like the rest.
The 12 Kotlin files under `shared/src/main/kotlin/com/vaadin/browserless/mocks` become 14 Java files. First phase of removing Kotlin from `shared`; the plan for the rest is in `de-kotlin-plan.md`. `MockHttpEnvironment.kt` splits into `MockHttpEnvironment`, `MockServletConfig` and `MockUtils`, and the top-level helpers of `MockVaadinServlet.kt` become statics on `MockVaadinServlet`. Every class is marked internal, and the KDoc is rewritten as Javadoc: each mock now says which container behaviour it reproduces and where it stops. Kotlin properties are kept as accessors rather than flattened into public fields, so most call sites are untouched — including the Kotlin ones, which see a Java getter/setter pair as a property. Breaking changes: * `MockVaadinServlet.createServletService` declares `throws ServiceException` and `createDeploymentConfiguration` declares `throws ServletException`, as `VaadinServlet` does. An override that calls `super` has to declare it too. * `MockHttpEnvironment.INSTANCE.setLocalPort(…)` becomes `MockHttpEnvironment.setLocalPort(…)`; same for `MockVaadinHelper`. * `MockHttpSession.Companion.create(…)` becomes `MockHttpSession.create(…)`. * `MockRequest.isUserInRole` takes a `BiPredicate` and the principal provider a `Supplier`, in place of the Kotlin function types. * The `HttpSession.attributes` extension is gone; use `new SessionAttributeMap(session)`. * `MockResponse` no longer exposes `_status`, `_bufferSize`, `_locale`, `_contentType` and `_characterEncoding`; use the `HttpServletResponse` getters and setters that back them. * The `WebBrowser(request)` factory is renamed `createWebBrowser(request)`. `MockInstantiator` is now `@Deprecated(forRemoval = true)` and forwards `getPageTitleGenerator()`, which the wrapper was missing.
The eight Kotlin utility files under `shared/src/main/kotlin/com/vaadin/browserless/internal` become nine Java files. Second phase of removing Kotlin from `shared`; `Locator`, `MockVaadin`, `PrettyPrintTree`, `Routes` and `Grid` are still Kotlin. See `de-kotlin-plan.md`. Each file becomes one utility class whose static methods take the former extension receiver as their first parameter. `TestingLifecycleHook` splits in two, since a Java interface cannot hold the mutable global the Kotlin top-level `var` provided: the interface keeps the hook methods and `TestingLifecycleHooks` holds the global and `cleanupDialogs()`. A 20-line `Matches.kt` stays behind for `Component.matches()` and `IntRange.size`, which take Kotlin-only types and go away with `SearchSpec` and `Grid`. Breaking changes: * The `*Kt` facade classes are gone: `BasicUtilsKt`, `ComponentUtilsKt`, `ElementUtilsKt`, `RenderersKt`, `ShortcutsKt`, `UtilsKt` and `DepthFirstTreeIteratorKt` become `BasicUtils`, `ComponentUtils`, `ElementUtils`, `Renderers`, `Shortcuts`, `Utils` and `DepthFirstTreeIterator`. * Property accessors lose their prefix along with the class rename, so `UtilsKt.getCurrentUI()` becomes `Utils.currentUI()`, `BasicUtilsKt.get_saneFetchLimit()` becomes `BasicUtils._saneFetchLimit()`, and `getId_` / `setId_` become the `id_` pair. * The global hook moves from a top-level property to `TestingLifecycleHooks.getCurrent()` and `setCurrent(…)`, and `cleanupDialogs()` to `TestingLifecycleHooks.cleanupDialogs()`. * `Component.isTemplate` becomes `TestingLifecycleHook.isTemplate(Component)`. * `findAncestor` and `findAncestorOrSelf` take a `Predicate` in place of the Kotlin function type. * `Button.caption` is folded into `caption(Component)`, which Kotlin resolved by static extension dispatch and Java cannot. * Helpers that were Kotlin `internal`, and so public only by accident of the bytecode, are package-private: `splitByWhitespaces`, `ellipsize`, `hasCustomToString`, `isRouteNotFound`, `getErrorParameterType`, `isEffectivelyVisible`, `isPolymerTemplate` and friends. * `serializeToBytes`, `deserialize` and `serializeDeserialize` are dropped; nothing called them.
Third phase of removing Kotlin from `shared`; `Locator`, `MockVaadin` and `Grid` are still Kotlin. See `de-kotlin-plan.md`. `MockRouteNotFoundError` and `MockInternalSeverError` get their own files, since Java allows one public class per file. `Routes` keeps the constructor overloads the Kotlin default arguments produced, and its sets are insertion-ordered as `mutableSetOf()` was, so the route list in a dump and in a `not found` message stays in a predictable order. `TreeOnFailureExtension` no longer goes through `PrettyPrintTree.Companion`. That line only runs when a test fails, so no suite would have caught it. Breaking changes: * `PrettyPrintTreeKt` and `RoutesKt` are gone: their functions are statics on `PrettyPrintTree` and `Routes`, and `PrettyPrintTree.Companion.ofVaadin(…)` becomes `PrettyPrintTree.ofVaadin(…)`. * `prettyStringHook` takes a `BiConsumer` in place of the Kotlin function type, and it and the other two globals move behind accessors: `getPrettyPrintUseAscii()`, `getPrettyStringHook()`, `getDontDumpAttributes()` and their setters. * `Routes` loses the members the `data class` generated: `component1()` through `component4()`, and `copy(…)` with defaulted arguments. The explicit four-argument `copy(…)`, `equals`, `hashCode` and `toString` remain. * `Routes` fields are behind accessors: `getRoutes()`, `getErrorRoutes()`, `getLayouts()`, `getSkipPwaInit()` and `setSkipPwaInit(…)`. * `MockRouteNotFoundError.cause` is behind `getCause()` and `setCause(…)`.
Fourth phase of removing Kotlin from `shared`; only `Grid.kt` and the two
helpers it still needs are left. See `de-kotlin-plan.md`.
`Locator.kt` becomes `Locator` plus `SearchSpec`, and `MockVaadin.kt` becomes
`MockVaadin` plus `SessionObjects`, `UIFactory`, `MockRequestCustomizer` and a
nested `MockPage`. A new `CountRange` replaces `kotlin.ranges.IntRange` in the
search spec, so no Java source in any module references a Kotlin type any more.
`MockVaadin` now fires session-init, service-destroy and UI-init through
`VaadinService.getEventBus()` with a handler that rethrows, so a listener
throwing during a test fails that test instead of only logging. It previously
reflected into the private listener collections.
The Kotlin `_get<Reified> { … }` syntax moves to a test-scope `LocatorDsl.kt` in
`junit6`, since Java has no equivalent of a reified type parameter or a receiver
lambda.
Breaking changes:
* `LocatorKt` and `MockVaadinKt` are gone; their functions are statics on
`Locator` and `MockVaadin`, taking a `Class<T>` and a
`Consumer<SearchSpec<T>>` in place of the reified type and the Kotlin block.
* `MockPage` moves from a top-level class to `MockVaadin.MockPage`.
* `SearchSpec` loses the all-argument constructor the Kotlin default arguments
produced; construct with `new SearchSpec(clazz)` and set what you need. Its
fields are behind accessors, `toPredicate()` returns a `Predicate`, and
`count` is a `CountRange`.
* `MockVaadin.userAgent` and `mockRequestFactory` are behind accessors, and the
factory is a `java.util.function.Function`.
* `MockVaadin.setup` and `setupServlet` take `Set<Class<?>>` rather than
`Set<? extends Class<?>>`, since Java has no declaration-site variance.
* The three `@Deprecated(forRemoval = true)` constructors taking
`Function0<UI>` are removed from `MockSpringServlet`,
`MockSpringServletService` and `MockSpringVaadinSession`. With a plain Java
`UIFactory` they are ambiguous against the `UIFactory` overload at any
`MockedUI::new` call site.
Final phase of removing Kotlin from `shared`. `shared/src/main` is now pure Java, `kotlin-stdlib` leaves its compile classpath, and consumers of `browserless-test-shared` pull no Kotlin runtime. See `de-kotlin-plan.md`. `Grid.kt` becomes `GridUtils` rather than `Grid`: a utility class of statics is not a `Grid`, and the old name shadowed `com.vaadin.flow.component.grid.Grid`, which forced 59 fully qualified references inside the file and 8 more in `GridTester`. The name follows the `BasicUtils` and `ComponentUtils` precedent. The `org.jetbrains.annotations` imports move to JSpecify, which Vaadin already provides, since those annotations arrived transitively with `kotlin-stdlib`. Dokka is replaced by `maven-javadoc-plugin`. Dokka validated nothing, so doclint now sees these sources for the first time and reports roughly 200 issues across 53 files, nearly all in testers untouched by this work. The module sets `doclint` to `none` so the port does not become a Javadoc cleanup; that cleanup is worth its own change. The other modules keep doclint on. Breaking changes: * `GridKt` is gone; its functions are statics on `GridUtils`. * `_rowSequence` returns a `Stream` in place of a Kotlin `Sequence`, still lazily, so `TreeGrid._size()` keeps counting without fetching every row. * `_dump(Grid, IntRange)` becomes `_dump(Grid, int from, int to)`. * The `KProperty1` overloads of `HeaderRow.getCell` and `FooterRow.getCell` are dropped; the `getCell(String)` overloads remain. * The intermediate `_clickItem` and `_doubleClickItem` arities the Kotlin default arguments generated are gone; the two-argument and the full-argument forms remain. * Property accessors lose their `get` prefix along with the class rename, so `get_internalId` becomes `_internalId` and `getDataProvider` becomes `dataProvider`.
The last 25 Kotlin files were all test-scoped, so this removes Kotlin from
the repository entirely: `kotlin-maven-plugin`, `dynatest`, `karibu-dsl`,
`kotlin-stdlib`, Dokka and the Spotless `kotlin` block are all gone from
the build.
DynaTest's `group { … }` / `test { … }` become `@Nested` / `@Test`, and its
helpers get Java equivalents in `TestSerialization` and `TestAssertions`.
`LocatorDsl.kt` existed only so Kotlin callers could write
`_get<Button> { caption = "…" }`; the tests now call `Locator._get(clazz,
spec -> …)`, which is what a user writes anyway. `AllTests.kt` was a pure
aggregator and disappears.
Test counts are unchanged across all five suites, verified by comparing the
surefire testcase names rather than the totals.
`shared` now builds clean under doclint, errors and warnings both, so
`<doclint>none</doclint>` is gone from its pom.
The "roughly 200 issues" that setting was added for was javadoc's output
cap, not the count: the tool stops after 100 errors and 100 warnings. The
real number was 935. The root pom now passes `-Xmaxerrs`/`-Xmaxwarns` so
the next reader gets a count rather than a ceiling.
Most of it was missing `@param`, `@return` and member descriptions in the
files the Kotlin port rewrote, since KDoc carries none of those tags. The
genuine defects were unresolvable `@link` and `@throws` targets, `<p/>`
and empty `<p>`, and markup that javadoc read as HTML.
`locator-processor` accounted for 291 of them, because it copies a tester
method's Javadoc into the generated `*Locator`. It now documents the
generated class's type parameters and both constructors, and rewrites a
copied `{@link #member}` to point back at the tester. The generated file
has no imports, so a tester has to spell cross-package `@link` and
`@throws` targets out in full; that rule is now in the guidelines.
Also converts the Javadoc the port left in KDoc markup, `[TreeGrid]` and
`*skip*`, to `{@link}`, `{@code}` and `<em>`.
The whole reactor now builds clean under doclint, errors and warnings
both.
Most of the 35 were the `AbstractBrowserlessExtension` builder helpers
and the undocumented default constructor of every extension, test base,
lookup initializer and security customizer; those classes now declare an
explicit constructor. The rest were the public fields and constructors of
the Spring mocks.
Also replaces a `{@link #reload()}` that pointed into a package-private
class, which javadoc cannot link to, with `{@code reload()}`.
`MockSpringServlet.routes`, `.ctx` and `.uiFactory` were public final fields. They are private now, behind `getRoutes()`, `getApplicationContext()` and `getUiFactory()` — the shape `MockVaadinServlet`, `MockService` and `MockVaadinSession` already use. Reading `servlet.routes`, `servlet.ctx` or `servlet.uiFactory` stops compiling; call the getter instead. There were no such callers in this repository, and the class is marked internal. `SessionObjects` gets the same treatment, but there it is a regression fix rather than a change: it was a Kotlin `data class`, so the released artifact exposes private fields plus `getSession()` and friends. The port to Java flattened the four `val`s into public fields while keeping the getters, which added API no release ever had. Its compiled surface now matches 1.2.0-beta1 again.
`GridContextMenuTester` and `GridTester.contextMenu` landed while
`shared` still had `<doclint>none</doclint>`, so their Javadoc was never
checked: 25 `<p/>` tags, which propagate into four generated `*Locator`
files, and a `{@link GridContextMenuTester#open()}` that does not resolve
once copied into a generated locator, since that file has no imports.
Also documents `GridUtils._internalId`, which became public for
`GridContextMenuSupport` to call from another package.
`MockVaadinTest.kt` carried five `/* TODO: uncomment after importing Locator APIs */` blocks, eight tests in all, and the port to Java dropped them silently. They were not running, so the testcase-name check that guarded the rest of the port could not see them. The Java `Locator` they were waiting on exists now, so they are back: navigation in a mocked environment, `beforeClientResponse` running once per lookup, a dialog's contents being reachable while it is open, reload re-navigating to the current URL, and the thread-pool example.
The conflict is javadoc in `GridTester.getCellText`: `main` rewrote the paragraph for the new `getCellComponent`, this branch had reflowed the same paragraph for doclint. Kept `main`'s wording with `<p>`. The `<p/>` that `main` added to `TextFieldTester` and `TextAreaTester` merged cleanly but no longer builds under doclint, since this branch dropped `<doclint>none</doclint>` from `shared`, so it is `<p>` here too.
The focus and blur simulation merged from main still called the Kotlin facade `PrettyPrintTreeKt`, which no longer exists on this branch.
Member
|
What impact would this change have on Kotlin users? |
Contributor
Author
|
It will for sure break compilation if they are using internal APIs or Kotlin extension methods like |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Removes Kotlin from the repository.
shared/src/mainand every test sourceare plain Java, and
kotlin-maven-plugin,kotlin-stdlib,dynatest,karibu-dsland Dokka are gone from the build, so consumers ofbrowserless-test-sharedno longer pull a Kotlin runtime.The working notes are in
de-kotlin-plan.md— phase byphase, the breaking changes, and a Kotlin→Java translation table.
Breaking changes
Everything below is in
com.vaadin.browserless.internal,.mocksor.component, or was already deprecated.*Ktfacade class disappears.UtilsKt→Utils,PrettyPrintTreeKt→PrettyPrintTree,GridKt→GridUtils, and so on.Since the class name changes anyway, the accessor names generated for Kotlin
properties are corrected at the same time:
getCurrentUI()→currentUI(),get_saneFetchLimit()→_saneFetchLimit(),getId_()/setId_()→id_()/id_(…).kotlin.ranges.IntRange→CountRangeonSearchSpec.countandint from, int toonGridUtils._dump;SearchSpec<T>.() -> Unit→Consumer<SearchSpec<T>>;Function0/Function1/Function2→Supplier/Function/Predicate/BiPredicate;KClass→Class<?>.ComponentQuery.withResultsSize(int),the documented entry point, is unaffected.
@Deprecated(forRemoval = true)Spring constructors are removed —the
Function0<UI>overloads onMockSpringServlet,MockSpringServletServiceandMockSpringVaadinSession. A JavaUIFactoryno longer extends
Function0, soMockedUI::newwas ambiguous against them.overridedeclares none even when the Java superclass does:
MockVaadinServlet.createServletServicedeclaresthrows ServiceExceptionand
createDeploymentConfigurationdeclaresthrows ServletException. Anoverride that calls
supernow has to declare the exception.MockSpringServlet.routes,.ctxand.uiFactoryare private, behindgetRoutes(),getApplicationContext()andgetUiFactory()— the shapeMockVaadinServlet,MockServiceandMockVaadinSessionalready use.Companion,INSTANCE,$defaultbridges,DefaultConstructorMarker, anddata classextras (componentN,copy$default).Tests
The 25 DynaTest/Karibu test files are JUnit 6 Java.
group/testmap onto@Nested/@Test; DynaTest'scloneBySerializationandexpectThrowshaveJava equivalents in
TestSerializationandTestAssertions;AllTests.ktwas a pure aggregator and is gone.
A green build does not prove a port kept its coverage, so each step was checked
by diffing the multiset of
<testcase name="…">values in the surefire XMLagainst a baseline, not the
tests="N"attribute, which DynaTestunder-reports. Against
mainthis branch is 1490 → 1498: 225 names renamedone-for-one from DynaTest's free text to the repository's
subject_scenario_expectationshape, plus 8 tests restored that the porthad dropped — they sat behind
/* TODO: uncomment after importing Locator APIs */, and the JavaLocatorthey waited for now exists.Javadoc
Replacing Dokka turns doclint on for
sharedfor the first time. Dokkavalidated nothing, so it had a backlog of 935 diagnostics — the number is
not the "roughly 200" an earlier note recorded, because javadoc stops
reporting after 100 errors and 100 warnings. The root pom now passes
-Xmaxerrs/-Xmaxwarnsso a count is a count.All of it is fixed, in
junit6,springandquarkustoo, and every modulenow builds clean under doclint with warnings included. 291 of the diagnostics
were in generated
*Locatorfiles and were fixed inlocator-processor,which also now rewrites a copied
{@link #member}to point back at the testerit came from. One rule for contributors came out of this and is in
guidelines/documenting.md: a tester has to spellcross-package
@linkand@throwstargets out in full, because its Javadoc iscopied into a generated locator that has no imports.
Two regressions this work found and fixed
VaadinSession.close()fires the UI detach listener twice. The Kotlintest used
var detachCalled = false, which hid it; ported to a counter itturned red. Left as-is and documented — worth its own look.
SessionObjectswas a Kotlindata class, so the released artifact exposesprivate fields plus
getSession()and friends. An earlier commit on thisbranch flattened the four
vals into public fields while keeping thegetters, adding API no release ever had. Reverted to private.
Verification
mvn clean install: 42 / 1456 / 4 / 41 / 31, all green.mvn clean install -DskipTests -Djavadocs: zero diagnostics.mvn spotless:check: clean.