Improve COCO parsing error messages

[zhiltsov-max]

Summary

• Extended dataset parsing error classification
• Added default string conversion for parsing errors
• Added COCO json parsing errors
• Added tests

How to test

Checklist

• I submit my changes into the develop branch
• I have added description of my changes into CHANGELOG
• I have updated the documentation accordingly
• I have added tests to cover my changes
• I have linked related issues

License

• I submit my code changes under the same MIT License that covers the project.
  Feel free to contact the maintainers if that's a concern.
• I have updated the license header for each file (see an example below)

# Copyright (C) 2021 Intel Corporation
#
# SPDX-License-Identifier: MIT

[IRDonch]

Most of these tests are very similar; I think it would be more readable to factor out the common parts into a single function that takes one parameter (a function that mangles the dataset) and returns the resulting exception. Then the individual tests could just look like this:

def mangle(anns):
    del anns["images"][0][field]

exc = self._test_mangled_annotations(mangle)
self.assertIsInstance(capture.exception.__cause__, MissingFieldError)
self.assertEqual(capture.exception.__cause__.name, field)

[zhiltsov-max]

Agree about the common part extraction, I'll check if it's possible.

[zhiltsov-max]

Done

[IRDonch]

You extracted a shared annotation template (which is certainly a big improvement), but there's still a lot of duplication in the structure of the tests. How about factoring that out too?

[zhiltsov-max]

How do you see that?

[IRDonch]

Like in my code snippet above. In full it would look something like this:

    def _load_mangled_annotations(self, mangle):
        with TestDir() as test_dir:
            ann_path = osp.join(test_dir, "ann.json")
            anns = mangle(deepcopy(self.ANNOTATION_JSON_TEMPLATE))
            dump_json_file(ann_path, anns)

            with self.assertRaises(ItemImportError) as capture:
                Dataset.import_from(ann_path, "coco_instances")
                    
            return capture

    def test_can_report_missing_item_field(self):
        for field in ["id", "file_name"]:
            with self.subTest(field=field):
                def mangle(anns):
                    anns["images"][0].pop(field)

                exc = self._load_mangled_annotations(mangle)
                self.assertIsInstance(capture.exception.__cause__, MissingFieldError)
                self.assertEqual(capture.exception.__cause__.name, field)

[zhiltsov-max]

Maybe, but it seem to save 1 line of code per test. It also requires to catch DatasetImportError and check the type and message in the test.

[IRDonch]

IMO, it would be useful to have a test that loads the unmodified template, to ensure that any errors in the other tests are due to the modifications made in those tests and not due to the template itself.

[zhiltsov-max]

The fact that the common piece was possible to extract is occasional, it is not something designed or expected. There are lots of tests that cover successful loading.

[IRDonch]

The fact that the common piece was possible to extract is occasional, it is not something designed or expected.

How is that not expected? The point of the new tests is to make sure that specific errors in the dataset cause the extraction to fail. To be certain that it's those errors that cause the failure, we need to ensure that the original template contains no errors.

[zhiltsov-max]

It is checked by the error details checks. However, I can agree, that such a test could be useful on its own, because there is no such test for just a json parsing.