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

Automate testing for GPT with mock implementation #21

Merged
merged 10 commits into from
Aug 13, 2024
Merged

Automate testing for GPT with mock implementation #21

merged 10 commits into from
Aug 13, 2024

Conversation

ByteYJ
Copy link
Collaborator

@ByteYJ ByteYJ commented Aug 6, 2024

This updates automating testing for ocr functionality and mock api call, including tests for:

Part 1: Testing llm_ocr_function
test_parse_table_data : ensures correct extraction and conversion of table data into DataFrames.
test_rescale_image : verifies the image resizing functionality for both largest and smallest dimensions.
test_encode_image : checks the base64 encoding and decoding of images.
test_correct_image_orientation : validates the correction of image orientation based on EXIF data.

Part 2: Testing OpenAI API call
test_query_api : verifies successful API responses using mocked responses.
test_query_api_authentication_error : tests handling of authentication errors using mocked error responses.

(These tests do not include extract_text_from_image and related functions, which requires a real API key and focuses more on OpenAI's effectiveness testing.)

Copy link
Contributor

@ginic ginic left a comment

Choose a reason for hiding this comment

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

@ByteYJ , I think the OpenAI mocks are off to a good start, but we should actually be testing our own code somehow with this. For example, we should add a test for key functions in src/msfocr/llm/ocr_functions.py, like get_results or parse_table_data. Without checking those functions, it's kind of just an example on how the OpenAI API works, which isn't really the purpose of a unit test. Remember that our goal is to test the code that we're writing and make sure they work correctly in normal situations (OpenAI returns a correct json response) and edge cases (OpenAI returns an error or malformed json). The thing you want to mock is the request to the OpenAI client and force it to return the response you need for the test.

Could you try to add a test for one of those functions that uses your mocked classes in one of those situations?

Some other things you might want to watch out for:

  • You're installing the pytest_mock dependency, but it's not used in the tests right now. Instead you're using the built-in unittest library, which is an alternative to the pytest library we're using for our tests. I think this is okay and they should work together, but I haven't tried it before, although there is a reference on https://docs.pytest.org/en/7.1.x/how-to/unittest.html.
  • While having logging is a good idea later on, we don't need logging in the unit test files, that can be removed.

@ByteYJ
Copy link
Collaborator Author

ByteYJ commented Aug 7, 2024

@ginic thanks for the suggestions! I added more tests to ocr_functions. now it includes six test cases.

Comment on lines +172 to +189
with Image.open(image_path) as image:
image.load()

orientation = None
try:
for orientation in ExifTags.TAGS.keys():
if ExifTags.TAGS[orientation] == 'Orientation':
break
exif = dict(image.getexif().items())
if exif.get(orientation) == 3:
image = image.rotate(180, expand=True)
elif exif.get(orientation) == 6:
image = image.rotate(270, expand=True)
elif exif.get(orientation) == 8:
image = image.rotate(90, expand=True)
except (AttributeError, KeyError, IndexError):
pass
return image
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a heads up, I was wrong about the correct way of handling the image earlier. I changed it again and I think this is correct now. You can read more at https://pillow.readthedocs.io/en/latest/reference/open_files.html#file-handling

Copy link
Contributor

@ginic ginic left a comment

Choose a reason for hiding this comment

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

Thanks, @ByteYJ ! This look great! I'm really glad we have tests for these things now.
If you wanted to take things one step further, you could use your OpenAI mocker to test the extract_text_from_image function, but I think that's going above and beyond, so only do that if you feel like you have time.

@ByteYJ ByteYJ merged commit 34232bb into main Aug 13, 2024
5 checks passed
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.

2 participants