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

[JBTM-3395] fixes of failures on the basic IT tests #1728

Merged

Conversation

ochaloup
Copy link
Contributor

https://issues.redhat.com/browse/JBTM-3395

!MAIN !CORE !QA_JTA !QA_JTS_JDKORB !QA_JTS_OPENJDKORB !QA_JTS_JACORB !BLACKTIE !XTS !PERF NO_WIN !RTS !AS_TESTS !TOMCAT !JACOCO
LRA

Copy link
Contributor

@mmusgrov mmusgrov left a comment

Choose a reason for hiding this comment

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

Damn, I've just finished fixing this because it was causing one of my fixes to fail.

You need to be checking for NOT_FOUND. Also the TCK already tests it so it doesn't belong here. If fixed it as follows:

    // test that an active LRA is reported correctly and then test that a closed LRA is reported correctly
    // Note that the TCK already has tests that start and close LRAs so this so this one needs removing
    @Test
    public void testGetAllLRAs() {
        URI lra = lraClient.startLRA("test-lra");

        List<LRAData> allLRAs = lraClient.getAllLRAs();
        Assert.assertTrue(allLRAs.stream().anyMatch(lraData -> lraData.getLraId().equals(lra.toASCIIString())));

        lraClient.closeLRA(lra);

        try {
            Assert.assertNotEquals(LRAStatus.Active, lraClient.getStatus(lra));
        } catch (WebApplicationException e) {
            assertEquals("LRA should have finished",
                    Response.Status.NOT_FOUND.getStatusCode(),
                    e.getResponse().getStatus());
        }
    }

@mmusgrov
Copy link
Contributor

The fix I'm working on has about 4 commits in it and this fix is 2 commits back. If you can change your PR to remove the test then that will make it easier for me to fix up my own fix (or I can do it in mine).

@ochaloup ochaloup added the Hold label Nov 26, 2020
@ochaloup
Copy link
Contributor Author

@mmusgrov
Do you mean the fix for this issue will be part of the #1729 or some another upcoming PR?

I can close this PR if you have got a fix for this.
And the check Assert.assertNotEquals(LRAStatus.Active, lraClient.getStatus(lra)); sounds better than mine.
Just, do you plan to proceed with with that multi commit PR soon? Or would be possible to rip off just the fix for this and make a separate PR now?

@mmusgrov
Copy link
Contributor

mmusgrov commented Nov 28, 2020

Well I had already fixed it because it was blocking my multi commit PR #1729 and it has been merged with that PR. But I also suggested that we already have tests (and one for close in the same file on line #L114) for this functionality so I was really suggesting that we simply delete the test.

@xstefank
Copy link
Collaborator

The idea of that test is to test lraClient.getAllLRAs() not start/stop.

@ochaloup
Copy link
Contributor Author

@mmusgrov ok, I see, I didn't got that. I'm closing the PR and the issue.

About the deleting the test I agree with @xstefank as the test checks the functionality of the LRAClient method and it should not be deleted. In contrary more tests should be added as we lack the test coverage of the LRAClient.

@ochaloup ochaloup closed this Nov 30, 2020
@mmusgrov mmusgrov reopened this Dec 2, 2020
@mmusgrov
Copy link
Contributor

mmusgrov commented Dec 2, 2020

I reopened it because the fixes in the other PR (#1729) never got committed.

@mmusgrov mmusgrov removed the Hold label Dec 2, 2020
@ochaloup ochaloup merged commit 0e1a48e into jbosstm:master Dec 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants