Are suite files necessary for testng regression testing?

[vlsi]

It looks like the current pattern is to add test classes to the suites.

For instance, https://github.com/cbeust/testng/blob/40f7935b00d3546187f793e98d0f49ea41ef542d/testng-core/src/test/resources/testng.xml seems to list classes that are not really related. testng.xml adds no parameters, it adds no important before/after suite listeners, so it is not clear what are the benefits of having such a suite.

However, suites add the following issues:

1. The global suite file is prone to merge conflicts: different people might add unrelated tests to the same place in the suite file.
2. One can forget to include the newly added test class to the suite, so the test won't get executed at all.

So I would suggest avoiding the suites unless there's an explicit need for that.

[vlsi]

What if each test class could specify its "belongs to suite" property via annotation or something like that, so testng could get all the test classes, group them into suites, and run accordingly?

Then even a single class would be runnable from IDE since the runner would pick up the suite (to configure suite listeners, parameters), and it would look like a custom suite with a single file only.

[krmahadevan]

@vlsi - Am cross posting some of my observations from the github comment https://github.com/cbeust/testng/issues/2597#issuecomment-860071598

I dont think we have any of that convention yet in the codebase. Basically there are two types of tests.

1. Tests that are automatically eligible for running which when run can vet out a particular functionality. These are the regular TestNG tests without any extra requirement.
2. Tests that need to use the TestNG apis and which extend the test.SimpleBaseTest. This is like a helper base class that provides a lot of convenient functions for creating testng suite, testng test (<test> tag) etc., and which internally uses test classes (that look like test classes in point (1) but should not be run on their own but only via the test class (see org.testng.DryRunTest as an example

The testng suite basically refers to these two types of classes alone and as part of the review we check to ensure that the contributor has added a reference to src/test/resources/testng.xml

We have historically relied on the suite file as the one place to have all the tests run.

What if each test class could specify its "belongs to suite" property via annotation or something like that, so testng could get all the test classes, group them into suites, and run accordingly?

How do you propose we do this? Right now all tests of testng can be executed from the IDE because the IDE TestNG plugin takes care of running them and none of the tests that are classified as unit tests for the testng codebase have any special requirement of them needing any specific suite listeners or parameters etc.,

So a user can still have the test executed from the IDE by running them as they always do, and for the test to be invoked as part of the build process, then include a reference to it in the testng.xml file.

The global suite file is prone to merge conflicts: different people might add unrelated tests to the same place in the suite file.

The merge problem is not just isolated to the suite xml file, its also applicable to the CHANGES.txt. Infact I have seen PRs having merge conflicts more with CHANGES.txt rather than the TestNG suite xml file. Also I think when we do reviews for a PR we also catch

• missing test cases
• bad test cases
• test cases which were written but were not included in the testng.xml suite file.

@juherr Your thoughts on this ?

[vlsi]

We have historically relied on the suite file as the one place to have all the tests run.

Isn't ./gradlew test enough to run all the tests? What extra value does the suite add?

The merge problem is not just isolated to the suite xml file, its also applicable to the CHANGES.txt

CHANGES.txt does indeed induce merge conflicts, however, they are alleviated to a degree by merge=union attribute (see https://github.com/cbeust/testng/blob/2c3f38c074c9002f6280eb1e61530e2fcf9a8776/.gitattributes#L10). In other words, Git merge would combine lines instead of reporting "merge conflicts". That is not applicable for testng.xml since any automatic merge might result in unparseable xml.

The fewer points of contention the easier the contributions are, so having conflict on changes.txt does not justify adding more files that might become the source of merge conflicts.

How do you propose we do this?

If tests can be executed with ./gradlew test and ./gradlew test --tests org.testng.TestAbc, then why using the suite at all?

missing test cases bad test cases

It has nothing to do with suites. Of course, missing tests are bad, but suite does not solve that.

test cases which were written but were not included in the testng.xml suite file.

That would be a non-issue if there was no need to include tests to the suite.

[krmahadevan]

@vlsi

CHANGES.txt does indeed induce merge conflicts, however, they are alleviated to a degree by merge=union attribute

Please see this PR. https://github.com/cbeust/testng/pull/2594 It shows that there are merge conflicts on the CHANGES.txt. Does it mean that its not honouring this flag or is there something else that needs to be done ?

Isn't ./gradlew test enough to run all the tests? What extra value does the suite add?

Sure it would have been enough had we had some sort of a naming convention for distinguishing between build tests and samples test classes that a build test would use. Unfortunately that's not there today. The suite solves this problem.

Also the suite is one way of finding out what all tests are to be run without having to run them (A trivial advantage ofcourse).

Without having a naming convention defined for all new build tests (And applying a naming convention for the existing tests so that they get marked as build tests), we cannot get rid of the testng suite xml file.

Currently the suite xml is just an easier way of dodging this huge change and relying on addition of tests into the testng suite xml file.

[vlsi]

PR. #2594 It shows that there are merge conflicts on the CHANGES.txt

I guess the merge conflict is there since the PR trims spaces from various lines. It should merge fine if the PR just added the relevant changes.

Trim whitespace from the line ends should have been adressed by autostyle.

Without having a naming convention defined for all new build tests

I see, so the issue here is there are classes that look like valid TestNG tests, however, they are more like "input data" for the actual tests.

How about ingoring a subpackage like **/testdata/**/*.class and/or *TestData*.class? Of course it would require certain renames, however, migrating off suites would make it easier to add and run tests.

By the way, it is really puzzling that there are lots of classes like IssueTest that differ with package names only. When such test fails it is hard to understand what the test verifies, and it is hard to find the class. I know I can open the class by full name, however, it would be easier if class names were unique

[krmahadevan]

How about ignoring a subpackage like /testdata//*.class and/or TestData.class?

This would like I said need some sort of re-org with the tests because there's no fixed pattern of package names that exist today on which we can apply this.

The tests need to be first organized/grouped based on functionality and ideally speaking should have one single build test per functionality which would have many methods each of which test a specific aspect. This is unfortunately not present yet.

By the way, it is really puzzling that there are lots of classes like IssueTest that differ with package names only. When such test fails it is hard to understand what the test verifies, and it is hard to find the class.

True. I have been the primary person who has added many such test cases. Since we didn't have a predefined naming convention for build tests and test data test classes, I just chose to use the IssueTest as a name with a description attribute on the @Test annotation that explains the github issue (This again still doesn't explain what is being tested and so can/should be enhanced with atleast the method name explaining what is being done).

If we are to be moving towards getting rid of the suite file, then i believe that the first step towards that should be start re-organizing the tests by functionality or any such relevant grouping mechanism and in the process also rename the test class names.

[vlsi]

If we are to be moving towards getting rid of the suite file, then i believe that the first step towards that should be start re-organizing the tests by functionality or any such relevant grouping mechanism

I do not think there's a single "functional tree". It would probably work better if the tests were tagged appropriately.

However, I think the first step is to try running all test classes without suite and see what fails, and only then adjust what fails. That was the approach I followed to migrate pgjdbc from suites to the individual test classes.

[vlsi]

I just chose to use the IssueTest as a name with a description attribute on the @Test annotation that explains the github issue

Well, the convention in the description is not much different from the convention in the class name, so if you create a description, you can put almost the same text into the class name :)

[krmahadevan]

Well, the convention in the description is not much different from the convention in the class name, so if you create a description, you can put almost the same text into the class name :)

The description just had the github issue number, not what it does.

[vlsi]

The description just had the github issue number, not what it does

Then it is better to use annotations like @Issue rather than description

[krmahadevan]

@vlsi I am referring to the description attribute of the TestNG @Test annotation. We currently don't have a custom annotation that can link something back to github issues (and I dont think there's a need for that as well in TestNG codebase). Its more of being able to see the description (or) as a documentation purpose for someone who wants to get more context behind a particular test.

[vlsi]

It might be helpful to use Allure annotations (@Issue, @Feature) so tests have the appropriate metadata, and would help to get better test reporting by applying Allure report plugin.

[krmahadevan]

@vlsi - What would that help us do ? The TestNG dev doesn't look at the reports the way that an allure report plugin consumer is going to be using the reports. So IMO it's just an overkill for TestNG. We aren't looking at building a traceability report for any to see/review and then take action which is usually what the allure reports help a user with.

We just need to ensure that we do the following:

• Re-group the tests based on functionality
• Rename test classes and test methods such that its intent is clear to an end-user.

[juherr]

About the initial subject: is the suite file mandatory or not?

IMO, the suite file is a legacy and I'm not sure we can remove it without a big effort in the cleanup of the tests. The more recent ones are following the undocumented convention, that we are still building, but it is not the case for all. I suppose this convention doesn't cover all subjects and options.

I agree it could be a good objective to remove the suite file and let the build tool defining the suite itself. TBH, I have to comment the suite option in gradle, every time I want to debug a specific class because gradle or IntelliJ add an unwanted listener in the context...

@krmahadevan WDYT if we try to write and improve the convention of TestNG tests? @vlsi Could you start a document that lists all points we have to clarify?

---

About dedicated annotation vs attribute in the annotation, I think it was an @cbeust's opinionated option: I remember a blog article with a similar debate but I can't retrieve it.

For the moment, description is enough because we are not using its content.

[vlsi]

I'm not sure we can remove it without a big effort in the cleanup of the tests

We can add new tests to /testng module rather than to /testng-core, then newly added tests do not need suites.

At the same time, we can move tests from /testng-core to /testng so the refactoring does not need to be a single massive change.

every time I want to debug a specific class because gradle or IntelliJ add an unwanted listener in the context

Yes, that global always failing listener clutters test results.

[vlsi]

Could you start a document that lists all points we have to clarify?

I do not get the question. If you need a suite for some reason, it makes sense to clarify that. If you do not need the suite, then just create tests as usual without adding them to the suite, and that is it.

[vlsi]

As a datapoint, the following change in testng-core/build.gradle.kts is more-or-less fine to make ./gradlew test run without errors:

//        suites("src/test/resources/testng.xml")
        exclude("test/**")
        exclude("test_result/**")
        exclude("org/testng/internal/listeners/parameterindex/TestWithProviderTest*")
        exclude("org/testng/BasicSample*")
        exclude("org/testng/DryRunSample*")
        exclude("org/testng/internal/conflistener/Failing*")
        exclude("org/testng/internal/paramhandler/ParameterizedSampleTestClass*")

          0,2sec,    6 completed,   0 failed,   0 skipped, org.testng.internal.TestListenerHelperTest
          0,0sec,    3 completed,   0 failed,   0 skipped, org.testng.internal.dynamicgraph.EdgeWeightTestSample1
          0,1sec,  100 completed,   0 failed,   0 skipped, org.testng.internal.dynamicgraph.LotsOfEdgesTest
WARNING   0,7sec,  130 completed,   0 failed,   1 skipped, Gradle test
WARNING   0,7sec,  130 completed,   0 failed,   1 skipped, Gradle suite
WARNING   2,7sec,  260 completed,   0 failed,   1 skipped, Gradle Test Run :testng-core:test

BUILD SUCCESSFUL in 5s

[juherr]

Why not refactoring the test folder by applying the current testing convention? It will allow removing the suite file and cleaning the legacy tests.

[vlsi]

That's possible as well. My current stack is: "make my tests working the way I like" =it requires=> "make testng recognize scopes" =it requires=> "adding tests or something".

In practice, I went with adding testng-kotlin-tests module, so I do not need to deal with the current suites.