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

Cleanup tests #1856

Merged
merged 15 commits into from
Feb 23, 2021
Merged

Cleanup tests #1856

merged 15 commits into from
Feb 23, 2021

Conversation

wcekan
Copy link
Contributor

@wcekan wcekan commented Feb 21, 2021

Description

Cleanup test syntax for better readability.
Using RestAssured with matchers will generate better debug on failure.

Motivation and Context

Code quality.

How Has This Been Tested?

Unit tests

License

I confirm that this contribution is made under an Apache 2.0 license and that I have the authority necessary to make this contribution on behalf of its copyright owner.

@wcekan wcekan requested review from moizarafat and aklish and removed request for moizarafat February 21, 2021 12:20
@wcekan wcekan force-pushed the cleanup-tests branch 2 times, most recently from 4e1f910 to 3dfd234 Compare February 21, 2021 15:56
moizarafat
moizarafat previously approved these changes Feb 22, 2021
Copy link
Collaborator

@moizarafat moizarafat left a comment

Choose a reason for hiding this comment

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

LGTM

@wcekan wcekan force-pushed the cleanup-tests branch 2 times, most recently from 90b249e to 2a30f12 Compare February 23, 2021 00:17
assertEquals(getResult3, expected3);
assertEquals(getResult5, expected5);
assertEquals(getResult6, expected5);
.body(equalTo(expected5));
Copy link
Member

Choose a reason for hiding this comment

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

expected6?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, that is how it worked.

        assertEquals(getResult5, expected5);
        assertEquals(getResult6, expected5);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I assume it is making 2 different requests expecting the same results.

.contentType(JSONAPI_CONTENT_TYPE)
.accept(JSONAPI_CONTENT_TYPE)
.get("/filterExpressionCheckObj/3")
.then()
.statusCode(HttpStatus.SC_NOT_FOUND)
.extract().response().asString();
.statusCode(HttpStatus.SC_NOT_FOUND);
Copy link
Member

Choose a reason for hiding this comment

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

compare to expected4?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, there was no expected4. We could add one?

        assertEquals(getResult3, expected3);
        assertEquals(getResult5, expected5);
        assertEquals(getResult6, expected5);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would resemble

{
    "errors": [
        {
            "detail": "Unknown identifier 3 for filterExpressionCheckObj"
        }
    ]
}

@wcekan wcekan requested a review from aklish February 23, 2021 16:23
@aklish aklish merged commit 59f0c3c into master Feb 23, 2021
@aklish aklish deleted the cleanup-tests branch February 23, 2021 17:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants