Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Parameterized test silently fails when arguments are incorrectly defined #91

Closed
akutschera opened this issue May 1, 2023 · 7 comments
Closed
Assignees

Comments

@akutschera
Copy link
Collaborator

Given the following test

    @TestWithCases
    @TestCase(str1 = "foo", bool1 = true)
    @TestCase(str1 = "foo", bool1 = true)
    void xxx(String str, boolean val) {
        assertThat(val).isTrue();
    }

The test fails with

org.opentest4j.AssertionFailedError: 
Expecting value to be true but was false
Expected :true
Actual   :false

because there is nothing supplied for boolean val. The @TestCase annotation defined str1 and bool1 while it should have defined str1 and bool2, which leads to val having a default value of false.

What I would hope for is a message telling me something like 'cannot define first argument as string and boolean at the same timeornothing defined for second argumentor anything instead of just ignoring thebool1`.
The docs say that I cannot do it like this (which is good).
What I would like to discuss is if we should catch this mis-definition and tell the user about it instead of relying on the user to figure out what they mis-defined.

@Nylle
Copy link
Owner

Nylle commented May 1, 2023

Oh definitely! I'd love this to be an error, before executing the test - just like with customised fields that don't exist.

@Nylle
Copy link
Owner

Nylle commented May 1, 2023

Oh boy, that's a tricky one.

The problem is, that any annotation-method can only ever have a primitive type (including String and Class<T>), which is very unfortunate to begin with, because it massively limits the usefulness of TestCases.
On top of that, primitive types cannot be null, naturally, so they always need to have a default-value. This means, I don't know, how to detect whether one of the methods (str1() or bool2()) returns a set value or the default.
In fact, every TestCase, contains 6 default-values for all supported types, of which the ones specified in the annotation's parameters are overwritten.

In your test, when injecting the first argument, it will reflect the type (String) and execute the str1()-method on TestCase and get the configured value while for the second test-argument it will execute the TestCase::bool2()-method and receive its default-value.

The only hook on validation that I can see right now, would be finding out which arguments are actually configured in @TestCase(...) and so far I was not able to reflect that.

@jk-idealo jk-idealo self-assigned this May 3, 2023
jk-idealo added a commit that referenced this issue May 3, 2023
Duplicate customisations for the same argument will throw an exception.

Refs: #91
@jk-idealo
Copy link
Collaborator

I opened a PR for a fix. However I'm not sure, the error message is clearly describing the problem:

Duplicate customisation for test-method arguments at position 1, 2, 3, 4, 6

Any ideas for improvements?

@akutschera
Copy link
Collaborator Author

akutschera commented May 5, 2023

Maybe. When I change the test above to

    @TestCase(bool1 = true)
    @TestCase(bool1 = true)
    void xxx(String str, boolean val) {
        assertThat(val).isTrue();
    }

it will still fail with the PR as it is. I don't know how you can catch that when just validating the annotation. If you move the check to where you actually supply the values to the test parameters, this will probably work.
As a bonus, you could then construct an error message like
Duplicate customisation for test-method arguments at position 1, want String, also got boolean which would be a bit more verbose.
I would exit the checks on the first error. If you have multiple test cases that are mis-configured then you will have to re-run the tests multiple times until you fix them all.

@Nylle
Copy link
Owner

Nylle commented May 5, 2023

Good idea, I'll check this out.

Nylle added a commit that referenced this issue May 5, 2023
This required to move validation from constructor to getter.

Refs: #91
Nylle added a commit that referenced this issue May 5, 2023
Duplicate customisations for the same argument will throw an exception.

Refs: #91
@Nylle
Copy link
Owner

Nylle commented May 5, 2023

I completely screwed the rebase, so I re-created the branch and opened another PR.

Nylle added a commit that referenced this issue May 5, 2023
akutschera pushed a commit that referenced this issue May 5, 2023
Duplicate customisations for the same argument will throw an exception.

Refs: #91
akutschera pushed a commit that referenced this issue May 5, 2023
@jk-idealo
Copy link
Collaborator

Fixed in version 2.9.7.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants