testng-team / testng

TestNG testing framework
https://testng.org
Apache License 2.0
1.98k stars 1.02k forks source link

Unexpected behavior with abstract test classes #1222

Open juherr opened 7 years ago

juherr commented 7 years ago

Expected behavior

public abstract class AbstractClassSample {

  @Test
  public void a() {}

  @Test
  public void b() {}
}

The expected behavior is 2 skipped tests or a failed execution (because the user is providing an abstract class) In the case of a JUnit class, the behavior should be the same.

Actual behavior

The class is skipped and doesn't appear in any report. Only a warning is displayed with the appropriate log level: https://github.com/cbeust/testng/blob/9030a55ee018a564423b6a5c1801927a3da7dba9/src/main/java/org/testng/internal/TestNGClassFinder.java#L124 In the case of a JUnit class, both methods are failed

From: https://github.com/easymock/easymock/pull/186#discussion_r85662391

juherr commented 7 years ago

@cbeust What behavior do you prefer? Skipped tests, a failed execution or something else?

juherr commented 7 years ago

Ping @krmahadevan

krmahadevan commented 7 years ago

@juherr - A failed execution should usually indicate a problem with the test, but in this case, since TestNG is not able to instantiate the class to execute the tests, IMHO it would be better if we resort to skipped tests

cbeust commented 7 years ago

Agree with @krmahadevan.

martinandersson commented 7 years ago

Test methods in a public abstract superclass are not inherited by a concrete subclass. This causes no tests to be found:

package one;
public abstract TonsOfTestMethods {
    @Test
    public void test_one() {}
}

package one;
public ConcreteTest extends TonsOfTestMethods {
}

To resolve the issue and make the above ConcreteTest execute, the abstract superclass must 1) have his public access modifier removed, or 2) the class must be annotated @Test.

This represents two problems because not always is the abstract superclass colocated with the concrete subclass. I.e., the public access modifier must be put in place. Plus it seems like the semantics of @Test is multi-faceted and context dependent - that is, a bit confusing. I would have expected that @Test on class-level implicitly mark all public methods of the class therein as test methods but other than that, to have no other effects.

Bottom line is that when I look at TestNG's documentation, inheritance is basically not touched. Maybe we want to specify and document inheritance-related behavior. My advice is to please not enforce rules just because we today are unable to forsee all possible designs and use cases of the future. A package-location requirement of a an abstract superclass would be one such thing.

Finally, I added this comment here instead of filing a new issue simply because I think these issues are related in that they have the same cause, and the same solution. Instead of 1 "unexpected behavior", we now have at least 2. We should specify exactly how inheritance and abstract classes work in TestNG.

cbeust commented 7 years ago

Hi @MartinanderssonDotcom,

Inheritance is mentioned a few times in the documentation ("The annotations above will also be honored (inherited) when placed on a superclass of a TestNG class. ") so I'm a bit surprised that inheritance is not working with abstract classes. We'll look into it.

martinandersson commented 7 years ago

Yeah that's like the only place and it does not refer to inheritance of the @Test annotation =) abstract classes are also not documented.

Anyways, I forgot to say that a third solution to my former example would be to add an empty disabled test method in ConcreteTest .

Thank you for a great test framework! =)

juherr commented 7 years ago

@MartinanderssonDotcom Are you talking about junit tests which are the subject of the issue? If no and if want to talk about any improvement, then could you open a new issue?

martinandersson commented 7 years ago

Yes, unit test methods =) To quote myself:

Test methods in a public abstract superclass are not inherited by a concrete subclass.

juherr commented 7 years ago

@MartinanderssonDotcom The subject of this issue is JUnit, not "unit test". That's why I think you should open a new issue if you are not using the JUnit support of TestNG.

martinandersson commented 7 years ago

I'm sorry, the earth just stopped spinning. What are you talking about?? =)

juherr commented 7 years ago

@MartinanderssonDotcom Are you using Junit for the issue you discribed? If no, it is another issue and please, open a new issue with à runnable project that show the wrong behavior.

martinandersson commented 7 years ago

I am using TestNG. And I thought this was a tracker about TestNG?

juherr commented 7 years ago

Again, please open a new issue. This one is about the junit support of TestNG.

krmahadevan commented 7 years ago

@MartinanderssonDotcom - I tried to execute the sample that you shared in this comment. I am not able to reproduce the issue. FWIW I am using TestNG 6.11

Here's the sample and the output

import org.testng.TestNG;
import org.testng.annotations.Test;

import java.io.File;
import java.net.URL;

public class TonsOfTestMethods {
    @Test
    public void testOne() {
        URL url = TestNG.class.getProtectionDomain().getCodeSource().getLocation();
        String version = new File(url.getFile()).getParentFile().getName();
        System.err.println("Hello World from TestNG version : " + version);
    }
}
public class ConcreteTest extends TonsOfTestMethods { }

Output

mvn clean test -Dtest=ConcreteTest
[INFO] Scanning for projects...
[INFO]
[INFO] ------------------------------------------------------------------------
[INFO] Building testbed 1.0-SNAPSHOT
[INFO] ------------------------------------------------------------------------
[INFO]

-------------------------------------------------------
 T E S T S
-------------------------------------------------------
Running com.rationaleemotions.github.issue1222.ConcreteTest
Hello World from TestNG version : 6.11
Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 10.262 sec - in com.rationaleemotions.github.issue1222.ConcreteTest

Results :

Tests run: 1, Failures: 0, Errors: 0, Skipped: 0

[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
krmahadevan commented 2 years ago

@juherr - What should be done with this issue? Below is what I am currently seeing

Abstract base class

package issue1222.one;

import org.testng.annotations.Test;

public abstract class TonsOfTestMethods {
  @Test
  public void test_one() {}
}

Child class resides in a different package

package issue1222.two;

import issue1222.one.TonsOfTestMethods;

public class ConcreteTest extends TonsOfTestMethods {}

Test runner class

package issue1222;

import issue1222.one.TonsOfTestMethods;
import issue1222.two.ConcreteTest;
import org.assertj.core.api.Assertions;
import org.testng.TestNG;
import org.testng.annotations.Test;
import org.testng.internal.ExitCode;
import test.SimpleBaseTest;

public class IssueTest extends SimpleBaseTest {

  @Test
  public void runSubClassTests() {
    TestNG testng = create(ConcreteTest.class);
    testng.setVerbose(2);
    testng.run();
    System.err.println("Exit Code : " + testng.getStatus());
    Assertions.assertThat(testng.getStatus()).isEqualTo(0);
  }

  @Test
  public void runAbstractBaseClassTests() {
    TestNG testng = create(TonsOfTestMethods.class);
    testng.setVerbose(2);
    testng.run();
    System.err.println("Exit Code : " + testng.getStatus());
    Assertions.assertThat(testng.getStatus()).isEqualTo(ExitCode.HAS_NO_TEST);
  }
}

Given the fact that we are looking at deprecating JUnit support, please suggest as to what we should be doing with the issue (From TestNG perspective, I feel that the above sample works as per expectations i.e.,

juherr commented 2 years ago

The issue is when you don't have a child class and you only pass the abstract class in the suite.

Agree, we can ignore the junit part.

krmahadevan commented 2 years ago

@juherr

The issue is when you don't have a child class and you only pass the abstract class in the suite.

TestNG fails to find any test, when it is provided with an abstract base class (Even though the class has tests, TestNG cannot instantiate it because its an abstract class) and the test case gets skipped

Are you suggesting any other behaviour here?

juherr commented 2 years ago

I re-read the original exchange and the issue was only when the class is junit related.

Maybe we can improve the error message or close this issue by linking it to the issue about removing junit support.