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

Warning: Redundant recording and confused cascading #352

Closed
Milbor opened this Issue Oct 10, 2016 · 4 comments

Comments

2 participants
@Milbor

Milbor commented Oct 10, 2016

I am trying to remove this warning from our project when migrating from JMockit 1.25 to 1.28.
Should we use returns() instead of result field, is this the preferred way? Warning also disappears when removing result field but I think for clear statement of intention, it's better to specify return value explicitly.
I was thinking, what happens if another mocked instance is added to test without result field (default cascading), and found this bug:

public class SpikeTest {
    @Injectable
    private Manager manager;
    @Injectable
    private Entity first;
    @Injectable
    private Entity second;

    interface Manager {
        Entity find();
    }

    interface Entity {
        Long getId();
    }

    @Test
    public void test() throws Exception {
        new Expectations() {{
            manager.find();
            //result = second;

            first.getId();
            result = 17;
        }};
        final Entity actual = manager.find();
        assertSame(actual, second);
        assertEquals(first.getId(), Long.valueOf(17));// fails, is 0
        assertEquals(second.getId(), Long.valueOf(0));// fails, is 17
    }
}

test fails with result field commented out, passes when present and also passes with returns() used.
How does the cascading logic work when multiple mocked instances are present as shown in this test? Test confirms, that second is used, but why? Is it because it is declared after first, i.e. the last declared instance in test class is used? I would expect some kind of non-unique exception thrown in this situation.

@rliesenfeld rliesenfeld self-assigned this Oct 10, 2016

@rliesenfeld rliesenfeld added the bug label Oct 10, 2016

@rliesenfeld

This comment has been minimized.

Show comment
Hide comment
@rliesenfeld

rliesenfeld Oct 10, 2016

Member

Thanks, it's a bug.

The "redundant recording" warning occurs when there is a single mock in scope that can be used as a cascaded return. The intended way to avoid it, in this case, is to not record the expectation. The warning should also occur if "returns(mock)" is used, I will fix that.

In the case there are two (or more) mocks of the cascading return type, the warning does not occur, since as you noticed the user should explicitly record which one is to be returned. There is a bug if the expectation is recorded but without a return value. JMockit is using the "last" mock available for cascading; I will consider throwing an IllegalStateException instead, to force the explicit recording of one of the possible mock instances.

Member

rliesenfeld commented Oct 10, 2016

Thanks, it's a bug.

The "redundant recording" warning occurs when there is a single mock in scope that can be used as a cascaded return. The intended way to avoid it, in this case, is to not record the expectation. The warning should also occur if "returns(mock)" is used, I will fix that.

In the case there are two (or more) mocks of the cascading return type, the warning does not occur, since as you noticed the user should explicitly record which one is to be returned. There is a bug if the expectation is recorded but without a return value. JMockit is using the "last" mock available for cascading; I will consider throwing an IllegalStateException instead, to force the explicit recording of one of the possible mock instances.

@Milbor

This comment has been minimized.

Show comment
Hide comment
@Milbor

Milbor Oct 11, 2016

If we do not record the expectation to remove the warning, we won't get MissingInvocation exception if code under test does not execute the expectation. So we lose this kind of assertion.
Documentation section Regular versus strict expectations:
If no invocation matches a given recorded expectation, a "missing invocation" error gets thrown at the end of the test

Is there other solution to remove this warning and to keep missing invocation check? StrictExpectations are too much, because they assert order. It seems this warning tries to forbid previously legit Expectations usage.

Milbor commented Oct 11, 2016

If we do not record the expectation to remove the warning, we won't get MissingInvocation exception if code under test does not execute the expectation. So we lose this kind of assertion.
Documentation section Regular versus strict expectations:
If no invocation matches a given recorded expectation, a "missing invocation" error gets thrown at the end of the test

Is there other solution to remove this warning and to keep missing invocation check? StrictExpectations are too much, because they assert order. It seems this warning tries to forbid previously legit Expectations usage.

@rliesenfeld

This comment has been minimized.

Show comment
Hide comment
@rliesenfeld

rliesenfeld Oct 11, 2016

Member

The test could have a new Verifications() {{ manager.find(); }} block then. But, really, would that be necessary? If the code under test uses the return from a matching find call, then usually that's sufficient to ensure everything is properly exercised and verified. Otherwise, the test is likely over-specifying things.

Member

rliesenfeld commented Oct 11, 2016

The test could have a new Verifications() {{ manager.find(); }} block then. But, really, would that be necessary? If the code under test uses the return from a matching find call, then usually that's sufficient to ensure everything is properly exercised and verified. Otherwise, the test is likely over-specifying things.

@Milbor

This comment has been minimized.

Show comment
Hide comment
@Milbor

Milbor Oct 12, 2016

What if there is another manager method with same return type? Then invocation assertion is desired.
Prior to this redundant warning, Expectations could be used to assert expected invocation together with return value. Seems like now non-redundant recordings can stay in Expectations block and redundant has to be moved to Verifications block. Isn't this change of behavior for the worse? Thank you.

Milbor commented Oct 12, 2016

What if there is another manager method with same return type? Then invocation assertion is desired.
Prior to this redundant warning, Expectations could be used to assert expected invocation together with return value. Seems like now non-redundant recordings can stay in Expectations block and redundant has to be moved to Verifications block. Isn't this change of behavior for the worse? Thank you.

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