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

Testing MessageValidator::lintMessage method #44

Merged

Conversation

Djuki
Copy link
Contributor

@Djuki Djuki commented Oct 6, 2016

Improving test coverage #39

I tested the method MessageValidator::lintMessage, and this PR is not ready for the test since test is not green.

I will explain why. I tested only this method, isolated, that is why I only test his responses.

If the message is valid lintMessage will return an empty string, if it is not valid it will return string error message. NOTE: It will not throw an Exception, which can be expected IMHO. This is why MessageValidator::lintMessage can't catch the \Seld\JsonLint\ParsingException and this method always return an empty string, even for not valid messages.

Method lint in \Seld\JsonLint\JsonParser class will return null if message is valid, and if message is not valid it will return Exception object.

We can talk about the design of this classes if you want, or we can just discuss expectation in test method invalidJsonMessage. I expect to get the error message if the message is not valid. If would be better to throw the exception instead of returning error string IMHO.

What are next steps about this issue?

@Djuki Djuki changed the title Testig MessageValidator::lintMessage method Testing MessageValidator::lintMessage method Oct 6, 2016
@kocsismate
Copy link
Member

Thank you really much for this detailed explanation! And I like the way you use data providers, I haven't been aware of them until now :) I'll try to answer appropriately during the weekend.

@kocsismate
Copy link
Member

kocsismate commented Oct 13, 2016

OK, at last I had time to understand the problem. I am glad that you chose this class to test and found important issues here. :)

You are absolutely right, the linter should throw an exception. But the thing is, the MessageValidator::lintMessage() method is only used by two classes which extend MessageValidator and they will emit an exception correctly. That means MessageValidator::lintMessage() should be protected instead (I can't imagine why I left it as public). In this case I think it is OK for it not to raise an exception.

Furthermore, MessageValidator::lintMessage() will return null if the message is empty which behaviour is really silly, so an empty string would be the appropriate return value.

What do you think about my reasoning?

If you still have some time in the near future then I will happily wait for you to finish the PR, otherwise I can also take it over from you.

@kocsismate kocsismate added this to the 1.0 milestone Oct 14, 2016
@Djuki
Copy link
Contributor Author

Djuki commented Oct 17, 2016

Ok. I will prepare new changes and unit tests soon.

@Djuki Djuki force-pushed the djuki-message-validator-tests/master branch from 28c6672 to 0a111bb Compare October 17, 2016 13:07
{
if (empty($message) === true) {
return null;
return "";
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kocsismate MessageValidator is changed by your request. All methods are protected, and we do not need to test this class anymore.

@@ -105,7 +105,7 @@ public function validateQueryParams()
protected function isValidMediaTypeHeader($headerName)
{
$header = $this->getHeaderLine($headerName);
return (strpos($header, "application/vnd.api+json") === false || $header === "application/vnd.api+json");
return strpos($header, "application/vnd.api+json") !== false;
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 had to fix this method since it was returning true for any kind of header. After this change, it returns TRUE only if the header line is application/vnd.api+json. If this is how it should behave than my change is correct.

@kocsismate
Copy link
Member

Thank you for your work! I'll merge your changes and take care of the rest! Have a nice Hacktober! :)

@kocsismate kocsismate merged commit 88e6c80 into woohoolabs:master Oct 17, 2016
@Djuki Djuki deleted the djuki-message-validator-tests/master branch October 19, 2016 08:58
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.

2 participants