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

[JSTEP-10] Migrate from using /failing tests to /tofix tests #1372

Conversation

JooHyukKim
Copy link
Member

Resolves #1370

@JooHyukKim JooHyukKim changed the title Migrate from using /failing tests to /tofix tests [JSTEP-10] Migrate from using /failing tests to /tofix tests Dec 30, 2024
* {@link JacksonTestFailureExpected} annotation.
*/
@Override
public void interceptTestTemplateMethod(Invocation<Void> invocation,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This API is for @ParameterizedTest.
For every method invocation, this method is called.

Comment on lines +55 to +61
ExpectedPassingTestCasePredicate predicate = findzExpectedPassingTestCasePredicate(invocationContext);
if (predicate != null) {
if (predicate.shouldPass(invocationContext.getArguments())) {
// do-nothing, we do not expect an exception
return;
}
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What we are trying to do here is use ExpectedPassingTestCasePredicate to allow passing test being part of test cases of provided by @ParameterizedTest this way we can keep test cases in one place (like @MethodSource)

@JooHyukKim
Copy link
Member Author

@cowtowncoder Now that we have additional implementation like one here, around @ParameterizedTest, now I feel more need for the test jar deployment you suggested.. But still hold the same about the timing(when to do)

try {
invocation.proceed();
} catch (Throwable t) {
// do-nothing, we do expect an exception
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there an expectation of always getting an exception, to indicate failing test? I guess this is true in that assert-methods do throw assertion exceptions.

implements ExpectedPassingTestCasePredicate
{
@Override
public boolean shouldPass(List<Object> arguments) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not so sure about this -- seems bit fragile and not very easy to understand. Is something like this expected for all "tofix" parameterized tests?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh. Looks like this is just one case, maybe there is something special about that one.

import java.util.List;

/**
* Interface that defines a predicate to determine if a test case is expected to pass.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume "pass" here mean that test completes without exception? (if so, fine, if not should probably be explained further).

@cowtowncoder cowtowncoder merged commit 4ac3a40 into FasterXML:2.19 Dec 31, 2024
8 checks passed
@JooHyukKim JooHyukKim deleted the 1370-migrate-from-failing-tests-to-tofix-tests branch January 8, 2025 16:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Convert handling of failing tests (JSTEP-10)
2 participants