Split modules #2564

vlsi · 2021-05-28T14:32:32Z

Note: below tricks might help to hide file movements:
A) GitHub UI: scroll the diff ("files changed") to the very end, and type the following to the browser console: $$('div.empty').forEach(function(e) { e.parentElement.parentElement.style.display='none'; })

B) You can watch individual commits (e.g. open Commits ) and review individual ones

C) You can use git log -M30% -l0 --stat -p -G. on the command line to show non-trivial diffs only (hide movements)

D) You can use git diff -M30% -l0 --stat -p -G. origin/master to see the full diff

Note GitHub has a limit on the number of renamed files, so sometimes it shows a file as "deleted and inserted" even if that was a simple rename.

-l0 can be used to set "unlimited" number of "renamed files" in git diff / git log.

The true diff (excluding file movements) is 85 files changed, 1617 insertions(+), 812 deletions(-)

Open questions:

none

Probably resolved:

Should testng-* be published (see Split modules #2564 (comment))? -- not for now
Should multi-module publishing be handled now? (see Split modules #2564 (comment)) -- publishing to Central is included
Should we deal/prepare to split packages issues now? Answer: no, we could shuffle classes later.
Which other classes should be moved to API (or their own modules). Answer: see above, we could shuffle classes later.

TODO:

juherr

This change is really awesome because I've already tried before without success due to my lack of knowledge of gradle.

Thanks a lot! 🙏

build-logic/publishing/src/main/kotlin/testng.maven-publish.gradle.kts

core-api/src/main/java/org/testng/IModuleFactory.java

core-api/src/main/java/org/testng/internal/Utils.java

core-api/src/main/java/org/testng/internal/objects/InstanceCreator.java

core-api/src/main/java/org/testng/internal/reflect/ReflectionHelper.java

core-api/src/main/java/org/testng/reporters/FileStringBuffer.java

juherr · 2021-05-28T15:44:43Z

versions.properties

@@ -0,0 +1,73 @@
+#### Dependencies and Plugin versions with their available updates.
+#### Generated by `./gradlew refreshVersions` version 0.10.0


I didn't know it before, it looks awesome!

@krmahadevan FYI https://jmfayard.github.io/refreshVersions/update-dependencies/

juherr · 2021-05-28T15:48:18Z

Just need to fix the build and add an entry in the CHANGES file ;)

vlsi · 2021-05-28T16:51:37Z

@juherr , well, you are optimistic :)

What I did was I pulled org.testng.annotations to api, and then I pulled all the transitive dependencies.
However, it might be there are still classes in core that need to be in the API.

For instance, org.testng.Assert should probably be in the API.

On the other hand, it is not clear what is the nature of org.testng.TestNG. It looks like a client-facing API, however, it depends on a lot of internal classes.

Then, the change creates "split package" issue where org.testng package is split across two artifacts :(

vlsi · 2021-05-28T16:52:45Z

PS. @juherr , @krmahadevan , do you prefer ant, api, bom, core, core-api folder naming or testng-ant, testng-api, testng-bom, testng-core, testng-core-api?

juherr · 2021-05-28T18:15:06Z

well, you are optimistic :)

Yes!! 🤩

For instance, org.testng.Assert should probably be in the API.

Agree, or in a testng-assert module (because we recommend 3rd party assertion library) if you have a solution for a all-in-one-testng-jar

On the other hand, it is not clear what is the nature of org.testng.TestNG.

It is the entry point for integrations: ant, maven, gradle, intellij, eclipse, netbeans and maybe more (like our own tests) use it to run tests.

PS. @juherr , @krmahadevan , do you prefer ant, api, bom, core, core-api folder naming or testng-ant, testng-api, testng-bom, testng-core, testng-core-api

I think testng- prefix will help to distinct testng module folders

juherr · 2021-05-28T18:23:39Z

I'm really excited about these changes! 😍

We use gradle for a while now but the move from Maven is still difficult.
Could you explain in few words your gradle strategy?
Why do you have many files? What are their purposes?

vlsi · 2021-05-28T18:26:15Z

We use gradle for a while now but the move from Maven is still difficult.

Could you please clarify? AFAIK Maven is no longer used for building TestNG.

Could you explain in few words your gradle strategy?
Why do you have many files? What are their purposes?

If you like, we can setup a small (Zoom?) call so I can walk you through the idea behind the change.
It might be a nice meetup for TestNG community as well.

juherr · 2021-05-29T01:25:41Z

Could you please clarify? AFAIK Maven is no longer used for building TestNG.

We moved to Gradle but we still don't manage it very well 🤷‍♂️

If you like, we can setup a small (Zoom?) call so I can walk you through the idea behind the change.

Good idea!

juherr · 2021-05-29T01:53:21Z

Which other classes should be moved to API (or their own modules)?

Maybe reporter/reporter-api or some others but could be done later because the current change is already big enough without too much code changes.

vlsi · 2021-05-29T09:35:14Z

the current change is already big enough without too much code changes

Well, having several modules helps to validate the idea and it helps to figure out better naming.
For instance, with 2-3 modules the need for testng- prefix was not that obvious.

Now it looks like the gain here is as follows:

Split code into modules with proper dependencies. That allows a better separation of concerns.
Combine the modules into a single org.testng:testng jar just like before. It allows the clients to still use org.testng:testng dependency as usual. Of course, if they use a single jar they won't get much benefit, however, the jar would fulfill backward compatibility.
Later the clients could switch to fine-grained dependencies, so the ones who write tests can skip TestNG's assertions, and they do not have TestNG class on the classpath/autocomplete.
Sometime later "split package" could be healed somehow. For instance, the current org.testng is split across lots of modules which creates issues for both OSGi and JPMS.

juherr · 2021-05-29T11:08:04Z

I validate the plan!

About the modules, I don't know what will be the best cut but last time I tried, I had:

junit4 -> enable the junit4 feature
mustache -> in order to extract it outside the core because I've never seen someone using it
reporters -> implementations of the reporter API (there will be a cycling dependency with core)
runners/ant -> ant support
runners/cli -> cli support (with jcommander dependency)
runners/java -> api for integration
xml -> XML to model parser
yaml -> yaml to model parser

xml and yaml could be grouped into the same parent but I don't find a good name for the parent

Some of these modules need code refactoring, that's why I'd prefer to introduce them only once the structure with only-moved-classes-not-refactoring will be validated and merged :)

Ping @krmahadevan Could you have a look at the current change and tell us if you agree with the objective?

vlsi · 2021-05-29T11:30:54Z

Some of these modules need code refactoring, that's why I'd prefer to introduce them only once the structure with only-moved-classes-not-refactoring

That's my understanding as well. For now, I do only code movement with minimal cleanups.

vlsi · 2021-05-29T21:21:02Z

Yaml depends on XmlMethodSelector via testDescription.addPropertyParameters("method-selectors", XmlMethodSelector.class);; XmlMethodSelector uses MethodGroupsHelper.findGroupTransitiveClosure, and MethodGroupsHelper is quite an involved class :(

It looks like yaml module requires refactoring, so I would skip it for now :-/

vlsi · 2021-05-29T22:11:53Z

JUnitTestMethod extends BaseTestMethod, and BaseTestMethod queries MethodHelper.calculateMethodCanonicalName which depends on a lot of things.

It looks like junit module requires refactoring as well :-/

vlsi · 2021-05-29T22:53:28Z

Sample chart.

Legend:

plain line is api dependency (~Maven's compile), it will be visible on the compile classpath when someone uses testng artifacts
dashed line is implementation (~Maven's runtime). It won't be visible on the compile classpath for the users
dotted line is for features (~Maven's optional). The feature itself is not a module

krmahadevan · 2021-05-30T10:03:46Z

I validate the plan!

About the modules, I don't know what will be the best cut but last time I tried, I had:

junit4 -> enable the junit4 feature

mustache -> in order to extract it outside the core because I've never seen someone using it

reporters -> implementations of the reporter API (there will be a cycling dependency with core)

runners/ant -> ant support

runners/cli -> cli support (with jcommander dependency)

runners/java -> api for integration

xml -> XML to model parser

yaml -> yaml to model parser

xml and yaml could be grouped into the same parent but I don't find a good name for the parent

Some of these modules need code refactoring, that's why I'd prefer to introduce them only once the structure with only-moved-classes-not-refactoring will be validated and merged :)

Ping @krmahadevan Could you have a look at the current change and tell us if you agree with the objective?

@juherr - Am still trying to catch up with the updates on this. So please bear with me if the questions sound naive.

Can you please help me understand what usecases would the below modules solve for a user from being a TestNG user perspective ?

runners/cli
reporters
xml and
yaml

Because I am not sure I quite understand as to why do we need to get down to this level of a code break-up ?

Fundamentally speaking, what is the idea of breaking down the codebase into multiple modules aimed at solving ? Is it something like the spring framework providing multiple projects as individual components which a user can consume ?

If thats the idea, then I dont quite understand as to when would a user want to use just these modules ? They are in one or the other way tied to tesng-core.

I was hoping that we would have something much more macro level for e.g.,

runners/testng-junit
runners/testng-ant
runners/testng-java

Now I am still not sure how would we extract it, but a testng-core would also make sense.

The reports module in my opinion doesn't add a lot of value addition because we are looking at thinning down the number of default reporters that TestNG offers (beyond the textual, xml and emailable report). So why do we need to have just another module only for these 3 features (which I think is technically just 3 reporting classes) ?

vlsi · 2021-05-30T13:00:36Z

Fundamentally speaking, what is the idea of breaking down the codebase into multiple modules aimed at solving ?

a) For instance, if I want to create tests, I need @Test annotation, however, I do not want to bring all testng.internal and similar classes to my autocomplete menu.

b) Moving Assert to its own module would allow clients to skip it in case they use different assertion libraries. Even though having something out of the box is great, there might be valid reasons to use AssertJ, Truth, and other libraries.

c) Building separate modules helps to ensure the clients would be able to do the same. For instance, if they want to build TestNG suites from their custom formats, then they would need more or less the same APIs as the current yaml parser. However, it turns out even yaml parser uses a lot of internal classes. If yaml parser could live in its own module without depending on core, then it means there are enough APIs to build custom parsers outside of TestNG.

juherr · 2021-05-31T07:19:15Z

Can you please help me understand what usecases would the below modules solve for a user from being a TestNG user perspective ?

Fundamentally speaking, what is the idea of breaking down the codebase into multiple modules aimed at solving ?

My main goal is not for users but for the core team.
Currently, the base code has a lot of architecture issues like circular dependencies between packages/layers. Having a strong module separation instead of a weak package separation will help to highlight these issues.
At the same time, the core team is very little, and having little packages to maintain will allow us to be a focus on the main ones (and why not moving them outside the project later).
Having a strong separation allows better dependency management too.
In fact, I see a lot of advantages and no disadvantages.

Is it something like the spring framework providing multiple projects as individual components which a user can consume ?

It is more a side effect, but yes :) @vlsi already gives some use-cases.
To be honest, I think only the power users will use the modules.

If thats the idea, then I dont quite understand as to when would a user want to use just these modules ? They are in one or the other way tied to tesng-core.

First, a good practice is to separate API and implementation: testng-api + testng-core
But TestNG offers some SPI for specific features like injection (impl = testng-guice), reporting (impl = testng-reporters), reading suite (impl = testng-xml, tesntg-yaml), ...
We offer some ways to run testng too (impl = testng-ant, testng-cli, ...)

The reports module in my opinion doesn't add a lot of value addition because we are looking at thinning down the number of default reporters that TestNG offers (beyond the textual, xml and emailable report). So why do we need to have just another module only for these 3 features (which I think is technically just 3 reporting classes) ?

In my mind, we can group all default reporters into a single module. The users will have to choose.

krmahadevan · 2021-05-31T15:06:20Z

@juherr

Currently, the base code has a lot of architecture issues like circular dependencies between packages/layers. Having a strong module separation instead of a weak package separation will help to highlight these issues.

While I agree with you the circular layer, I am not sure if the package interdependencies is that big of a problem. Maybe that's the ignorance in me speaking.

At the same time, the core team is very little, and having little packages to maintain will allow us to be a focus on the main ones (and why not moving them outside the project later).

Yes this is the very same concern I have that if we end up having a lot of modules, then we may perhaps loose the benefits of multiple modules, because then release may become a problem (This may not be actually true, but just still calling this out)

First, a good practice is to separate API and implementation: testng-api + testng-core

When you say TestNG API, are you referring to the interfaces being created by TestNG and by implementation are you referring to the interface implementations ?

But TestNG offers some SPI for specific features like injection (impl = testng-guice), reporting (impl = testng-reporters),

I didn't quite understand this part. Can you please help explain this once more to me ?

juherr · 2021-05-31T20:33:53Z

I am not sure if the package interdependencies is that big of a problem

Right. It will depend on why and when you make packages.
I think @vlsi will confirm but packages are more important since JPMS

Yes this is the very same concern I have that if we end up having a lot of modules, then we may perhaps loose the benefits of multiple modules, because then release may become a problem (This may not be actually true, but just still calling this out)

Releases should not be more difficult than today: the same repository today and someone else problem if a module is dropped outside :)

First, a good practice is to separate API and implementation: testng-api + testng-core

When you say TestNG API, are you referring to the interfaces being created by TestNG and by implementation are you referring to the interface implementations ?

But TestNG offers some SPI for specific features like injection (impl = testng-guice), reporting (impl = testng-reporters),

I didn't quite understand this part. Can you please help explain this once more to me ?

Yes, testng-api is testng interfaces (for test descriptions and feature implementations aka SPI).
testng-guice is the implementation of the interfaces describing the injection feature.

vlsi · 2021-05-31T21:06:59Z

By the way, Guice injection is configurable (see https://github.com/google/guice/wiki/CustomInjections ), and it might be it is enough to cover all TestNG injection use cases.

krmahadevan · 2021-06-01T07:57:45Z

@juherr

Ok, I am not sure i still understand the broader picture. Since you have a much more clearer idea on this, i think i will side step and let you help drive this to closure, while i catch up on the concepts :)

juherr · 2021-06-01T10:47:06Z

@krmahadevan 👍

vlsi · 2021-06-01T21:47:36Z

@juherr , what do you think if individual jars are not published to Central until they are fully ready (e.g. split packages resolved)?

There might be options:
a. Publish both org.testng:testng (all modules) and org.testng:core-api (individual ones). It might result in classpath issues if different libs depend on both jars at the same time
b. Publish org.testng:testng as usual, and publish individual org.testng:core-api only for SNAPSHOT versions. Then the users could test individual jars in snapshot builds, however, it won't interfere with the release jar (since release version would come only in single jar variant)
c. Publish only single jar for now, and flip the switch sometime later (8.0? 9.0?) by replacing "single jar" org.testng:testng with pom-only dependency that references individual jars (~similar to what groovy-all does now)

vlsi · 2021-06-01T22:44:42Z

@juherr, do you manually close staging repositories at https://oss.sonatype.org/ ?

Frankly speaking, I use https://github.com/vlsi/vlsi-release-plugins/blob/master/plugins/stage-vote-release-plugin/README.md for publishing to Central.

Publishing multi-module projects would require https://github.com/gradle-nexus/publish-plugin to orchestrate the publishing, so we'd better configure the publishing even in case initially there will be only a single module published.

I can configure stage-vote-release-plugin here if you will, however, I am not sure if it would align with your workflows.

The key difference probably is that I tend avoid storing -SNAPSHOT versions in the repository and the plugin automatically sets release or -SNAPSHOT versions depending on -Prelease or -Prc parameters.
In other words, gradle.properties always contains a non-snapshot version, and -SNAPSHOT is dynamically added if needed.

The release procedure would be like:

./gradlew -Prc=1 prepareVote => build & push RC to staging repository
./gradlew -Prc=1 publishDist => promote the staged repository and push Git tag

I use the plugin for quite a while now in pgjdbc, Apache Calcite, Apache JMeter, etc.

juherr · 2021-06-02T07:25:32Z

@juherr , what do you think if individual jars are not published to Central until they are fully ready (e.g. split packages resolved)?

I think the option c is the best: publish all artifacts in the next major version

@juherr, do you manually close staging repositories at oss.sonatype.org ?

For the moment, yes.
To be honest, I think our release process doesn't fit the best practices. For example, we lost the git tag when we moved from maven to gradle.
I had some projects to check in my todo list but never found the time to dig:
https://github.com/gradle-nexus/publish-plugin/
https://jreleaser.org/
If it is easy and documented, feel free to change anything in the release process.
If it is possible, I prefer to do it into another PR because this one becomes very big ;)

vlsi · 2021-06-03T12:58:14Z

I've made a couple of corrections:

I fixed a duplicate META-INF/LICENSE.txt file in testng.jar
I moved more manifest properties to /gradle.properties

The resulting testng-7.5.0.jar seems to have similar to testng-7.4.0.jar contents

See the sample: https://docs.gradle.org/release-nightly/userguide/structuring_software_products.html https://docs.gradle.org/release-nightly/samples/sample_structuring_software_projects.html

…ules

…tNG (testng-core)

vlsi · 2021-06-03T14:46:01Z

Ok, I updated the changelog, and I even fixed OSGi headers. Not sure if testng works, however, previously it failed to load at all, and now it loads, and one can query its version via org.testng.internal.Version#getVersionString

juherr · 2021-06-03T19:55:25Z

I reviewed and see 3 little issues:

JUnitSample4 should import junit Assert
test111.Test1 should import testng Assert
unexpected copyright on DefaultTestngOsgiOptions and PlainOsgiTest

Now, I'm trying to load your branch before merging.

vlsi · 2021-06-03T20:13:28Z

JUnitSample4 should import junit Assert

What do you mean?
I replaced junit.framework.Assert with org.junit.Assert because junit.framework.Assert is deprecated.

test111.Test1 should import testng Assert

Same thing. I replaced junit.framework with org.junit.
If you think there was a bug, feel free to fix it (e.g. after merging the PR).
Of course, I can incorporate the change, however, it does not seem to be related to the PR.

unexpected copyright on DefaultTestngOsgiOptions and PlainOsgiTest

Well, I borrowed code from pgjdbc which is BSD-2-Clause. I believe it is compatible with testng.testng-osgi-test is not going to be released, so licensing should be quite clear.
However, I might need to add the license file itself just in case.

I can remove the code if you do not like borrowing.

PS. What do you think of testng-osgi-test? Would it better be renamed it to testng-it-osgi or testng-test-osgi? Then all "integration" tests would sort together.

juherr · 2021-06-03T20:26:30Z

What do you think of testng-osgi-test? Would it better be renamed it to testng-it-osgi or testng-test-osgi? Then all "integration" tests would sort together.

I vote for testng-test-osgi

fixes testng-team#2575

juherr approved these changes May 28, 2021

View reviewed changes

juherr requested a review from krmahadevan May 28, 2021 15:46

vlsi force-pushed the split_modules branch from 3b252bf to 121978a Compare May 28, 2021 18:46

vlsi force-pushed the split_modules branch from 1335122 to e7d3341 Compare May 31, 2021 21:01

krmahadevan mentioned this pull request Jun 1, 2021

Move Parser into being an internal API #2573

Merged

2 tasks

vlsi force-pushed the split_modules branch from 21bfa5c to defe660 Compare June 3, 2021 12:55

vlsi force-pushed the split_modules branch 2 times, most recently from 4f72621 to a0149ae Compare June 3, 2021 14:26

vlsi added 4 commits June 3, 2021 17:26

Add .editorconfig: trim whitespace, indent style=2 space

9962a41

Un-ignore gradle.properties

3f8d1d8

Cleanup use of deprecated APIs in tests

10adefe

Migrate to Gradle 7.0.2 + build-logic

7762598

See the sample: https://docs.gradle.org/release-nightly/userguide/structuring_software_products.html https://docs.gradle.org/release-nightly/samples/sample_structuring_software_projects.html

vlsi force-pushed the split_modules branch from a0149ae to d695c24 Compare June 3, 2021 14:28

vlsi added 8 commits June 3, 2021 17:38

Add api, bom, core, core-api, ant, asserts, collections, test-kit mod…

cd8988f

…ules

Remove reference from IInvokedMethodListener (testng-core-api) to Tes…

445f2f9

…tNG (testng-core)

Add testng module that merges testng jars

3b19f79

Make guice optional in api

5568251

Split test-runner-api module

a8962c9

Make testng-* non-published

7bc3f74

Move OSGi manifest to testng module

c82c2b9

Configure releases to Central

b997ee6

vlsi force-pushed the split_modules branch from be904b9 to ef12d02 Compare June 3, 2021 14:40

vlsi force-pushed the split_modules branch from ef12d02 to 9285e57 Compare June 3, 2021 20:17

vlsi added 2 commits June 3, 2021 23:35

Add testng-test-osgi to verify testng.jar is loadable in OSGi

b64f74c

fixes testng-team#2575

Update CHANGES.txt

47ffdca

vlsi force-pushed the split_modules branch from 9285e57 to 47ffdca Compare June 3, 2021 20:35

juherr merged commit 1ce27a3 into testng-team:master Jun 3, 2021

juherr mentioned this pull request Jun 3, 2021

Create core module #2311

Closed

vlsi deleted the split_modules branch June 3, 2021 22:22

vlsi mentioned this pull request May 30, 2022

Gradle build system assertj/assertj#277

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split modules #2564

Split modules #2564

vlsi commented May 28, 2021 •

edited

juherr left a comment

juherr May 28, 2021

juherr commented May 28, 2021

vlsi commented May 28, 2021

vlsi commented May 28, 2021

juherr commented May 28, 2021

juherr commented May 28, 2021

vlsi commented May 28, 2021

juherr commented May 29, 2021

juherr commented May 29, 2021

vlsi commented May 29, 2021

juherr commented May 29, 2021

vlsi commented May 29, 2021

vlsi commented May 29, 2021

vlsi commented May 29, 2021

vlsi commented May 29, 2021

krmahadevan commented May 30, 2021

vlsi commented May 30, 2021

juherr commented May 31, 2021

krmahadevan commented May 31, 2021

juherr commented May 31, 2021

vlsi commented May 31, 2021

krmahadevan commented Jun 1, 2021

juherr commented Jun 1, 2021

vlsi commented Jun 1, 2021

vlsi commented Jun 1, 2021

juherr commented Jun 2, 2021

vlsi commented Jun 3, 2021

vlsi commented Jun 3, 2021

juherr commented Jun 3, 2021

vlsi commented Jun 3, 2021 •

edited

juherr commented Jun 3, 2021

		@@ -0,0 +1,73 @@
		#### Dependencies and Plugin versions with their available updates.
		#### Generated by `./gradlew refreshVersions` version 0.10.0

Split modules #2564

Split modules #2564

Conversation

vlsi commented May 28, 2021 • edited

juherr left a comment

Choose a reason for hiding this comment

juherr May 28, 2021

Choose a reason for hiding this comment

juherr commented May 28, 2021

vlsi commented May 28, 2021

vlsi commented May 28, 2021

juherr commented May 28, 2021

juherr commented May 28, 2021

vlsi commented May 28, 2021

juherr commented May 29, 2021

juherr commented May 29, 2021

vlsi commented May 29, 2021

juherr commented May 29, 2021

vlsi commented May 29, 2021

vlsi commented May 29, 2021

vlsi commented May 29, 2021

vlsi commented May 29, 2021

krmahadevan commented May 30, 2021

vlsi commented May 30, 2021

juherr commented May 31, 2021

krmahadevan commented May 31, 2021

juherr commented May 31, 2021

vlsi commented May 31, 2021

krmahadevan commented Jun 1, 2021

juherr commented Jun 1, 2021

vlsi commented Jun 1, 2021

vlsi commented Jun 1, 2021

juherr commented Jun 2, 2021

vlsi commented Jun 3, 2021

vlsi commented Jun 3, 2021

juherr commented Jun 3, 2021

vlsi commented Jun 3, 2021 • edited

juherr commented Jun 3, 2021

vlsi commented May 28, 2021 •

edited

vlsi commented Jun 3, 2021 •

edited