Improve error message format

[phoebey01]

The error message currently printed out doesn't directly give user the information they need. For example, if a user changes the type of a schema, the user would see:

[MIS-E002] Changed type: #/paths//pet/{id}/put/parameters/1/schema/properties/age (documentation: https://swagger-spec-compatibility.readthedocs.io/en/latest/rules/MIS-E002.html)

The "#/path//..." is hard to follow and not providing much useful information. Users just need to know 1) the property that has changed, 2) the endpoint & the method that are being affected by the change, 3) the error rule and 4) the link to the documentation explaining the rule. Similar case for other error rules.
We could simplify and clarify the error message so that it's more user-friendly, similar to another widely-adopted library swagger-diff has done it.

[phoebey01]

I've been working on reformatting the message. For example MIS_E001 would now print out:

[MIS-E002] Changed type:
put /pets/{id}: age changed from integer to number
(documentation: https://swagger-spec-compatibility.readthedocs.io/en/latest/rules/MIS-E002.html)

instead of:

[MIS-E002] Changed type: #/paths//pet/{id}/put/parameters/1/schema/properties/age (documentation: https://swagger-spec-compatibility.readthedocs.io/en/latest/rules/MIS-E002.html)

I plan to submit a pull request fixing the issue.

[macisamuele]

@yyang08: you're definitely correct, having more human messages is something that we should look for.

Originally the tool was built internally to be embedded into CI systems, so we did not invest a lot of time on making the messages more readable (as we were mostly relying on the JSON output).

Let me know if you will provide a PR for this!

PS: Sorry for the huge delay on dropping a comment :)

[phoebey01]

@macisamuele I've already implemented this. However, fixing all the tests is a huge headache because they all expect to see an error message with reference like '#/paths//pet/{id}/put/parameters/1/schema/properties/{}'.format(reference). It's hard to change for each rule in the tests from the way it is now, https://github.com/Yelp/swagger-spec-compatibility/blob/master/tests/rules/added_enum_value_in_response_test.py#98, to something that
look like: message = "{} {}: new enum value {} in property {}\n".format( enum_values_diff.path[2], enum_values_diff.path[1], enum_values_diff.mapping.new, enum_values_diff.path[-1])

Do you have suggestion for this?

[macisamuele]

@yyang08 without checking the code it would be a bit hard to say.
Definitely I do expect some complexity in updating all the tests to reflect the new format.

A possible approach might be by using mocks, we might:

• mock AddedEnumValueInRequest.validation_message (or equivalent)
• ensure that the method is called with the expected parameters
• the result (AddedEnumValueInRequest.validate) contains exactly the expected reports (mocked_menthod.return_value)
  And add a unit-test around the validation_message to ensure that it works as we would expect.

Something that I would suggest is to create a PR with what you already have.
It will not be merged (make sure to mention that is a WIP) but it will give us a common place where to contribute, I might try to allocate some time to help you out fix all the tests.

[phoebey01]

@macisamuele I created a PR and you can see the file changes here (https://github.com/Yelp/swagger-spec-compatibility/pull/15/files). Though I dont have much experience with unit testing in python, I think mocking validation_message would work.

[macisamuele]

@yyang08 the PR is a starting point for understanding how to update tests.
Something that I would suggest is to keep PRs small, so test failures are easier linkable to a specific change 😎
Tomorrow I'll check the PR out and try to give some examples on how testing might be possible.

---

I dont have much experience with unit testing in python

Something that I did used to get a better understanding of mocks was https://realpython.com/python-mock-library/.