testng icon indicating copy to clipboard operation
testng copied to clipboard

Possible to exclude the testng-assert module when using testng

Open mpet opened this issue 1 year ago • 26 comments

TestNG Version

v7.10.2

Suggestion

Hi In our team we discussed this: Our majority of TestNG testcases uses https://joel-costigliola.github.io/assertj/ for assertions. But when users write TestNG unit tests the first alternative that pops up is org.testng.Assert. Would it be possible to make this module optional? This is especially important for users that use TestNG for executing tests and assertj to assert.

br,

//mikael

mpet avatar Dec 13 '24 10:12 mpet

Currently, TestNG is released into a single artifact, but the project is already modularized and assertions are in their own module. Maybe we should consider releasing TestNG artifact without assertions plus an extra artifact for assertions.

@krmahadevan WDYT

Related to #2649

juherr avatar Dec 13 '24 10:12 juherr

@juherr - That would mean that we would need to do 3 artifact releases

  • Assertions library from TestNG
  • TestNG without assertions
  • TestNG with assertions (it would consume the assertion library)

Is this what we are looking at ?

krmahadevan avatar Dec 13 '24 10:12 krmahadevan

Only assertions and testng without assertions.

It means the upgrade will need to add assertions as dependencies if used.

juherr avatar Dec 13 '24 10:12 juherr

Wouldn't that cause backward compatibility issues for users? Yes, we can send out announcements etc., but just calling out.

Should we do this as part of a minor release or a major release?

krmahadevan avatar Dec 13 '24 10:12 krmahadevan

I agree, release the assert library as a separate artifact. Mark it as optional dependency in the pom.xml that user can add if they need it.

martinaldrin avatar Dec 13 '24 12:12 martinaldrin

@krmahadevan yes, it will make a little regression that must be documented

juherr avatar Dec 13 '24 15:12 juherr

@juherr is it accepted?

mpet avatar Dec 20 '24 06:12 mpet

@mpet I think it will accepted when someone will propose a pull request for that. 😉

juherr avatar Dec 20 '24 06:12 juherr

@juherr I guess we need to exclude the asserts from shaded jar too. Maybe it should not be included at all. What do you say?

mpet avatar Dec 20 '24 07:12 mpet

@mpet excluded from the shaded jar and deployed as a standalone artifact.

juherr avatar Dec 20 '24 07:12 juherr

ok. @juherr Any input on what should be in the build file for testng-asserts. I guess some of the stuff in https://github.com/testng-team/testng/blob/master/build.gradle.kts should be reused.

mpet avatar Dec 20 '24 08:12 mpet

I cannot see where the 'publish' is done

So I would need your assistance of what parts to include the ..\git\testng\testng-asserts\testng-asserts-build.gradle.kts since it is not a regular release.

I have added this, so far:

plugins {
    id("testng.java-library")
    id("maven-publish")
}

group = "org.testng" 
version = "7.8.0"
description = "TestNG Assertions Library"

dependencies {
    implementation(projects.testngCollections) {
        because("Lists.newArrayList")
    }

    testImplementation("org.testng:testng:7.3.0") {
        because("we need testng to test assertions")
    }
    testImplementation(projects.testngTestKit)
}



publishing {
    publications {
        create<MavenPublication>("mavenJava") {
            from(components["java"])
            pom {
                name = "TestNG Assertions"
                description = "Assertions library for TestNG"
                url = "https://testng.org"
                licenses {
                    license {
                        name = "Apache License, Version 2.0"
                        url = "http://www.apache.org/licenses/LICENSE-2.0.txt"
                    }
                }
                developers {
                    developer {
                        id = "testng-developers"
                        name = "TestNG Developers"
                        email = "[email protected]"
                    }
                }
                scm {
                    connection = "scm:git:git://github.com/testng-team/testng.git"
                    developerConnection = "scm:git:ssh://[email protected]/testng-team/testng.git"
                    url = "https://github.com/testng-team/testng"
                }
            }
        }
    }

What am I missing?

How can I test a release and make sure it has the correct info?

mpet avatar Dec 20 '24 09:12 mpet

@krmahadevan do you know the release process and which parts that I need?

mpet avatar Dec 20 '24 10:12 mpet

@mpet - The release process is documented here: https://github.com/testng-team/testng/wiki/TestNG-release-process

I believe that we would perhaps need to create a github actions workflow that would release assert library.

@vlsi - It would be good if you could help share some guidance around here.

krmahadevan avatar Dec 20 '24 11:12 krmahadevan

yes it seems to have a dedicated flow where you set the RC to be released. Not sure what is need in the build to support this. But I hope @vlsi can shed some light in this matter...

mpet avatar Dec 20 '24 14:12 mpet

@krmahadevan is @vlsi still active? How can we make this go forward...

mpet avatar Dec 23 '24 08:12 mpet

@mpet - Vlad helped setup the build system for TestNG. He can help more aptly. Given that this is a holiday season I would expect latency in people responding.

I am not well versed with Gradle as a build system so I don't have an answer and will need to spend time trying to figure out.

So we wait till @vlsi gets back or you can try taking a jab at this.

krmahadevan avatar Dec 23 '24 08:12 krmahadevan

The key question is the way to handle backward compatibility.

We could move asserts to a different artifact, and use it as a regular dependency. Then, at some later version (major one?), the dependency could be removed, so the ones who need it should add it manually.

The sad thing is that ideally, different jars should not have the same packages. Currently, Assert lives in org.testng which is the same package that keeps many other useful interfaces like ITestContext and so on.

Unfortunately, the migration from org.testng.Assert to something like org.testng.assertions.Assert would be a bit more complicated (and it would probably require adding an artifact with a different name.


In other words:

Current state:

TestNG 7.10.2 testng.jar "everything in a single jar"

TestNG 8.0.0 testng.jar "except assertions" testng-assertions.jar "org.testng.Assert"

TestNG 9.0.0 testng.jar "except assertions" testng-assertions.jar "org.testng.Assert as deprecated". There could be automatic migrations like OpenRewrite rewrite-testing-frameworks or InlineMe testng-assertions2.jar "org.testng.assertions.Assert"

TestNG 15.0.0 testng.jar "except assertions" drop testng-assertions.jar testng-assertions2.jar "org.testng.assertions.Assert"


WDYT?

vlsi avatar Dec 23 '24 08:12 vlsi

Thanks for the feedback. I think it could go faster

What if "org.testng.assertions.Assert" is introduced and "org.testng.Assert" is deprecated in the next minor release?

What if "org.testng.assertions" is moved into its dedicated artifact, and core depends on it in the next minor release?

Both moves should not break anything, right?

juherr avatar Dec 23 '24 13:12 juherr

What if "org.testng.assertions.Assert" is introduced and "org.testng.Assert" is deprecated in the next minor release?

Sure it is possible to combine the changes. I just assumed it would take some time to add org.testng.assertions, etc, etc.


By the way, my plan missed a release that would exclude "testng-assertions" from the default dependency of testng.jar. Frankly, I am not sure if moving org.testng.Assert to its own jar is backward-compatible as the users might have used something like -classpath testng.jar

vlsi avatar Dec 23 '24 14:12 vlsi

Right. It can break with cli runner.

Then I propose to create the new artifact with the new package, and deprecate assertions from the base package in a next minor version.

And remove deprecated in the next major as usual.

Wdyt @krmahadevan ?

juherr avatar Dec 24 '24 05:12 juherr

@juherr

So in TestNG 7.11.0 -

  • We deprecate org.testng.Assert
  • We create a new gradle module named testng-asserts
  • introduce org.testng.assertions.Assert in testng-asserts
  • publish testng-asserts as a separate artifact into Maven Central
  • Make this announcement as part of the release notes and release publishing activites.

In TestNG 7.13.0 we remove org.testng.Assert from the testng uber jar and be done with this activity.

Is this a fair understanding of the expectations here?

krmahadevan avatar Dec 24 '24 06:12 krmahadevan

In TestNG 7.13.0 we remove org.testng.Assert from the testng uber jar and be done with this activity

I would ask you not doing so. It takes time for the consumers to adapt, so "removal" should better be delayed by 5 or so years.

vlsi avatar Dec 24 '24 06:12 vlsi

@vlsi Personally I wouldn't bother removing Asserts at all. A test runner is supposed to provide the basic assertion capabilities. If someone doesn't want to use them then they can always use something else. It's just a matter of preference.

But the ask in this issue itself was that, they felt it was not an optimal experience. If we are going to keep the assertion classes for that long then we might as well not remove them 🫣

I don't have a strong opinion on this either way because there are straightforward ways of avoiding it, ranging from code reviews that enforce this avoidance to using the enforcer plug-in to ensure that the testng powered asserts are not used in the code base.

I was merely trying to figure out the list of activities that need to be done ( along with some timelines ) to accomplish the ask in this issue.

krmahadevan avatar Dec 24 '24 07:12 krmahadevan

@vlsi we have a large code base with many maven modules where we use testng as the testing fwk. However there is a mix of asserts libraries used. Even if we recommend using assertj various IDE:s propose different asserts. People also copy paste code.

So we basically want to clean code. Module by module and use one type of assert, assertj. So what @juherr suggested seems very reasonable and would support us on our work to remove testng asserts. It would also give complie time error when users use testng asserts in the future. So this change would be greatly appreciated.

mpet avatar Dec 28 '24 07:12 mpet

I'd recommend instead maintaining this company policy by including common IDE configurations through build or repository, so that it is applied at project import. See Eclipse's Type Filters and IntelliJ's Auto-Import Excludes. Then use an lint rule (an enforcer plugin, checkstyle, etc) to ban the import during the build so that it doesn't sneak through.

ben-manes avatar Dec 30 '24 19:12 ben-manes