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

#679: Add on fail message builder for response specification #1131

Closed
wants to merge 2 commits into from
Closed

#679: Add on fail message builder for response specification #1131

wants to merge 2 commits into from

Conversation

Frechet
Copy link
Contributor

@Frechet Frechet commented Feb 7, 2019

This is useful when you need to associate a mismatch description with additional user information. For example, in parallel execution.

@johanhaleby
Copy link
Collaborator

Is it important that the failure message is a part of the AssertionError?

I've made some changes where I'm using a ResponseValidationFailureListener to print the error message instead. I like the idea that we're building on existing building blocks (ResponseValidationFailureListener in this case) instead of implementing completely new concepts. On my machine it now looks like this:

image

However since describedAs is a part of the AssertionError it probably makes more sense the way you did it. Just want to hear your thoughts.

@Frechet
Copy link
Contributor Author

Frechet commented Feb 8, 2019

@johanhaleby Thank you so much for your quick feedback!

Is it important that the failure message is a part of the AssertionError?

I think, yes. I will try to explain why.
I looked at your proposed solution. Unfortunately, it cannot solve our problem. The fact is that in the ResponseValidationFailureListener we can write to the stream, but this will also be independent of the assertion written to the stream, as if we had written something like this:

log.info("test1");
response.then().statusCode(200)
--
[TestNG-tests-3] INFO test1
[TestNG-tests-2] INFO test3
// Here ResponseValidationFailureListener wrote something, but we cannot know it for this assert or another.
[TestNG-tests-4] INFO test2
...
java.lang.AssertionError: 1 expectation failed.
Expected status code <200> but was <404>.
[TestNG-tests-7] INFO test12

@mih-kopylov
Copy link

@johanhaleby any chance to have this PR merged?

@hasanalpzengin
Copy link

Important feature. What's status ? Do you need testers here ?

@MRC81
Copy link

MRC81 commented Jun 17, 2021

I guess many would be happy to have this feature! Any chances to see it merged?

@cheparsky
Copy link

@Frechet There is a conflict, please resolve it, maybe after that @johanhaleby merge it

@Frechet
Copy link
Contributor Author

Frechet commented Sep 14, 2021

Hello everybody! Thanks for your feedback! I will try to resolve the conflict as soon as possible and ask for a merge.

@Frechet
Copy link
Contributor Author

Frechet commented Sep 14, 2021

@cheparsky , @johanhaleby resolved conflicts #1502

@Frechet Frechet closed this Sep 14, 2021
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.

None yet

6 participants