AB-330: Class name range check #254
Conversation
|
Thanks for this patch. Also, good catch with the bad variables in the error message! 👏 Please make sure you read the CONTRIBUTING guidelines, especially the section about writing good commit messages. Make sure your commit messages and pull request descriptions clearly describe the change that you have made. |
| @@ -139,10 +139,10 @@ def validate_class(cls, idx=None, recordings_required=True): | |||
| # Name | |||
There was a problem hiding this comment.
We have some tests for the dataset validator at https://github.com/RashiSah/acousticbrainz-server/blob/b00f1f849ef2ba6041c308755c7de4dbcb90f699/utils/dataset_validator.py. It would be good to make sure that we have tests for these three methods that you changed so that we can be sure that they work as expected.
There was a problem hiding this comment.
@alastair
Can you please explain me a bit more about the tests you are talking about here: https://github.com/RashiSah/acousticbrainz-server/blob/b00f1f849ef2ba6041c308755c7de4dbcb90f699/utils/dataset_validator.py ?
There was a problem hiding this comment.
Whoops, I meant to say https://github.com/RashiSah/acousticbrainz-server/blob/b00f1f849ef2ba6041c308755c7de4dbcb90f699/utils/test/test_dataset_validator.py
It looks like we have tests for most things regarding the structure of the dataset dictionary, but we don't have any tests to check that an error is shown if the names of classes or datasets are longer or shorter than the given values. We should add 3 more tests that provide names that are too short, and too long, and check that the correct exception is raised.
There was a problem hiding this comment.
Added tests for the dataset or class names that are too short or too long than the specified ranges.
|
Great, thanks! Just as a note, you can make long strings in python by doing something like "a"*100which results in a string that is just as long (but maybe a little more boring than yours :) ) |
Changed the range check of higher value from MIN to MAX.