testng-team / testng

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

Parameterized test class crashes when data provider returns empty array #1030

Closed qerub closed 8 years ago

qerub commented 8 years ago

Let's say I have a test class with a @Factory-annotated constructor:

import org.testng.annotations.DataProvider;
import org.testng.annotations.Factory;
import org.testng.annotations.Test;

import static org.testng.Assert.assertEquals;

public class TestTest {
    @DataProvider(name = "values")
    public static Object[][] values() {
        return new Object[][]{{1}, {2}, {3}};
    }

    private final int value;

    @Factory(dataProvider = "values")
    public TestTest(int value) {
        this.value = value;
    }

    @Test
    public void test() {
        assertEquals(value, 2);
    }
}

This works perfectly (apart from the assertion errors this silly test throws).

However, if I change the data provider to return an empty array instead (new Object[][]{}), TestNG crashes:

org.testng.TestNGException: 
Can't invoke public void TestTest.test(): either make it static or add a no-args constructor to your class 

I expected the test to be ignored, just like it is when the data provider is on method-level instead of class-level. Is this the desired behaviour? If so, why? I ran into this issue because I have data providers that are supposed to be NOPs in certain test configurations.

juherr commented 8 years ago

How do you run the test?

qerub commented 8 years ago

Through Surefire and IntelliJ IDEA. The generated XML suite looks something like this:

<?xml version="1.0" encoding="UTF-8"?>
<!DOCTYPE suite SYSTEM "http://testng.org/testng-1.0.dtd">
<suite name="Default Suite">
  <test name="foo">
    <classes>
      <class name="TestTest"/>
    </classes>
  </test>
</suite>
juherr commented 8 years ago

Ok, thank :)

qerub commented 8 years ago

@juherr: Do you agree with me that this is a bug? (Just wonder if I should find another strategy or wait for a fix.)

juherr commented 8 years ago

@qerub I didn't try to reproduce it but it looks like a bug for sure. I won't have time to fix it soon. But if you are agree to try to fix it by yourself, I can help youto do it.

First step is to add a new tset case that proves the issue, then try to find where is the issue and fix it :)

priyanshus commented 8 years ago

@qerub Have you tried running your test programmatically? If yes, do you get the issue?

package test.testng1030;

import org.testng.annotations.DataProvider;
import org.testng.annotations.Factory;
import org.testng.annotations.Test;

import static org.testng.Assert.assertEquals;

public class TestEmptyReturnFromDataProvider {
    @DataProvider(name = "values")
    public static Object[][] values() {
        return new Object[][] {};
    }

    private int value;

    @Factory(dataProvider = "values")
    public TestEmptyReturnFromDataProvider(int value) {
        this.value = value;
    }
    @Test
    public void test() {
        assertEquals(value, 2);
    }
}
public static void main (String [] arg) {
        TestListenerAdapter tla = new TestListenerAdapter();
        TestNG testng = new TestNG();
        testng.setTestClasses(new Class[] { TestEmptyReturnFromDataProvider.class });
        testng.addListener(tla);
        testng.run();
    }

I tried, it does not throw any exception for me. Could you check it please.

juherr commented 8 years ago

Which version are you both using?

priyanshus commented 8 years ago

I have cloned repo yesterday, shows 6.9.12.

ryanlevell commented 8 years ago

@priyanshus Can you try with testng.setVerbose(2);? I get the same exception when I set verbose to 2.

public static void main(String[] args) {
    TestNG testng = new TestNG();
    testng.setVerbose(2);
    testng.setTestClasses(new Class[] { TestTest.class });
    testng.run();
}

Output:

[TestNG] Running:
  Command line suite

FAILED: test
org.testng.TestNGException: 
Can't invoke public void TestTest.test(): either make it static or add a no-args constructor to your class
...

I pulled from master today, but it says 6.9.11 in the pom. Gradle shows 6.9.12 though, is that where you found 6.9.12?

juherr commented 8 years ago

@ryanlevell Gradle is the official build tool, so the current master is 6.9.12(-SNAPSHOT)

priyanshus commented 8 years ago

@juherr What should be the intended behaviour if dataprovider returns an empty array? Should it throw an exception saying that you must have to provide values in dataprovider?

juherr commented 8 years ago

IMO, an empty dataprovider means "no case for the test" and should not fail (at least, not like observed). But a test with no case is a bit strange too. Maybe we can add it in the skipped list to notify we found a test which was not run.

What do you think @cbeust ?

cbeust commented 8 years ago

I'm not sure.

On one hand, I'd like to signal the user that something odd happened (empty data provider should be extremely rare) but on the other hand, marking this test SKIPPED might be a bit too strong but a simple log line might go unnoticed...

cbeust commented 8 years ago

Let's go for a log line, or a warning "Warning: the data provider returned an empty array so this test is not doing anything", but let's mark the test as a success.

Thoughts?

ryanlevell commented 8 years ago

Currently a test with an empty data provider returns 0 tests results, which seems intuitive to me, instead of marking the test as passed. I don't believe it has a log line though, which I agree should be added.

This issue only happens when combining an empty data provider with a Factory. Should it also just show 0 tests ran, or as you mentioned, make both situations show 1 test success?

priyanshus commented 8 years ago

@cbeust Without using @Factory for dataProvider test result look like this

Total : 0 Passed: 0 Failed : 0 Skipped : 0

Either we should fix both the cases or let it be same as without @Factory i.e. 0 result.

priyanshus commented 8 years ago

I tried debugging the issue but could not fix it.

Could not find the reason of having null value for testMthdInst.getInstance() at https://github.com/cbeust/testng/blob/d0fe7c63973110e78d55880ada7cc61035140345/src/main/java/org/testng/internal/TestMethodWorker.java#L112

And that is causing the exception. In case of non empty values the testMthdInst.getInstance() has some value.

ryanlevell commented 8 years ago

@priyanshus getInstance() is null because FactoryMethod#invoke returns an empty array when the data provider is empty.

This then prevents the loop in TestNGClassFinder from adding any instances.

Since this is the only place an instance would be found in this case, we have an instance test method with no instance that can be used to call it which causes Utils#checkInstanceOrStatic to throw the error.

I have code that fixes it, but it would also hide real user errors by preventing the error thrown by Utils#checkInstanceOrStatic. So I don't think it is a good solution yet.

@juherr do you know where I could go from here?

juherr commented 8 years ago

warning "Warning: the data provider returned an empty array so this test is not doing anything".

+

returns 0 tests results

in both cases (@DataProvider and @Factory).

@cbeust Agree?


@ryanlevell

I'm not sure TestMethodWorker is the good place to make the fix as it is common code with other ITestMethod.

Which line is calling Utils#checkInstanceOrStatic and then failing?

We must not forget that a factory could be a constructor too (so we need test it too).

juherr commented 8 years ago

Caution: since https://github.com/cbeust/testng/pull/1044 the behavior maybe changed

cbeust commented 8 years ago

@juherr LGTM