Skip to content

Athena: Added mock tests for the basic approach of the module_text_llm #88

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

Open
wants to merge 21 commits into
base: main
Choose a base branch
from

Conversation

alekspetrov9e
Copy link

Add tests for text LLM module basic approach

This PR adds e tests for the text LLM module's basic approach functionality. The tests verify the feedback generation process for student submissions using extensive mocking to isolate the functionality being tested.

Key features of the test suite:

  • Tests basic feedback generation with simple submissions
  • Tests handling of empty submissions
  • Tests processing of long input submissions
  • Uses mocks for OpenAI model configuration, NLTK sentence tokenization, and module configuration
  • Verifies proper feedback object creation and association with exercises/submissions

The tests ensure that the basic approach functionality works correctly across different scenarios while maintaining proper isolation from external dependencies.

@alekspetrov9e alekspetrov9e added the python Pull requests that update python code label Apr 7, 2025
@alekspetrov9e alekspetrov9e requested a review from a team as a code owner April 7, 2025 09:34
@github-actions github-actions bot added the athena label Apr 7, 2025
@alekspetrov9e alekspetrov9e changed the title Added some mock tests for the basic approach of the text module Athena: Added some mock tests for the basic approach of the text module Apr 7, 2025
@alekspetrov9e alekspetrov9e changed the title Athena: Added some mock tests for the basic approach of the text module Athena: Added some mock tests for the basic approach of the text module Apr 7, 2025
Copy link
Contributor

@ahmetsenturk ahmetsenturk left a comment

Choose a reason for hiding this comment

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

very nice addition, thank you @alekspetrov9e!
I have a small suggestion - I think you can modularize this code a bit, by adding maybe tests/utils and moving the mock models/environments/configs etc. there.
it would make it also easier to then write tests for other approaches 😊

Copy link
Contributor

@ahmetsenturk ahmetsenturk left a comment

Choose a reason for hiding this comment

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

Looks pretty nice 👍 Just added two nitpicks - other than that lgtm

Copy link

@LeonWehrhahn LeonWehrhahn left a comment

Choose a reason for hiding this comment

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

Currently, you're patching directly inside the test file, which is functional but clutters the test code and won’t scale well as the suite grows. I’d recommend moving these patches into fixtures in a conftest.py file. Since we’re likely to reuse much of this mocked functionality across different modules and tests, we should aim for a more reusable and modular setup.

There are two approaches I’d suggest considering:

  1. Centralized tests/ folder: We could move all tests under a single root-level tests/ directory. Inside that, we can organize shared fixtures, mocks, and test utilities in their own folders (e.g., tests/utils/, tests/fixtures/) and then have per-module test folders (like tests/module_text_llm/, etc.).

  2. Shared test_helpers/ module: Alternatively, we could create a shared test_helpers/ package to store common fixtures, mocks, and utilities. Each module would keep its own tests/ folder with local tests, but import shared components from test_helpers/.

In both setups, modules would import shared fixtures into their local conftest.py (or directly from the shared folder where appropriate) and can still keep their own specialised fixtures or mocks right next to their tests.

I personally lean toward option 1, but I think option 2 is equally valid depending on how modular we want to keep things.

What do you think @maximiliansoelch?

@maximiliansoelch
Copy link
Member

maximiliansoelch commented Apr 23, 2025

Currently, you're patching directly inside the test file, which is functional but clutters the test code and won’t scale well as the suite grows. I’d recommend moving these patches into fixtures in a conftest.py file. Since we’re likely to reuse much of this mocked functionality across different modules and tests, we should aim for a more reusable and modular setup.

There are two approaches I’d suggest considering:

  1. Centralized tests/ folder: We could move all tests under a single root-level tests/ directory. Inside that, we can organize shared fixtures, mocks, and test utilities in their own folders (e.g., tests/utils/, tests/fixtures/) and then have per-module test folders (like tests/module_text_llm/, etc.).
  2. Shared test_helpers/ module: Alternatively, we could create a shared test_helpers/ package to store common fixtures, mocks, and utilities. Each module would keep its own tests/ folder with local tests, but import shared components from test_helpers/.

In both setups, modules would import shared fixtures into their local conftest.py (or directly from the shared folder where appropriate) and can still keep their own specialised fixtures or mocks right next to their tests.

I personally lean toward option 1, but I think option 2 is equally valid depending on how modular we want to keep things.

What do you think @maximiliansoelch?

Generally, this is a valid point, and I am fine with both options. My only requirement is that the test files are clearly structured and identifiable to which module they belong, allowing for an easy modification/deletion of, for example, a deprecated module.

@alekspetrov9e
Copy link
Author

Thank you all for the reviews! I used the first approach that @LeonWehrhahn proposed and created a central tests folder and tests can be grouped by modules. I also resolved the two nitpicks from @ahmetsenturk :)

Copy link
Contributor

@ahmetsenturk ahmetsenturk left a comment

Choose a reason for hiding this comment

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

You should make use of the utils you have implemented - looks like you are defining the mock models you have already separated from the test suites

@maximiliansoelch maximiliansoelch changed the title Athena: Added some mock tests for the basic approach of the text module Athena: Added mock tests for the basic approach of the module_text_llm Apr 28, 2025
Copy link
Member

@maximiliansoelch maximiliansoelch left a comment

Choose a reason for hiding this comment

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

Minor thing in the test dependency declaration. See inline comment

Copy link
Member

@maximiliansoelch maximiliansoelch left a comment

Choose a reason for hiding this comment

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

Please also adapt the Github Action for testing to make use of your newly added test.
Additionally, please mark PR review suggestions that were resolved as such.

@alekspetrov9e alekspetrov9e requested review from a team as code owners May 2, 2025 14:31
@github-actions github-actions bot added the github label May 2, 2025
@github-actions github-actions bot removed the github label May 2, 2025
@github-actions github-actions bot added the github label May 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
athena github python Pull requests that update python code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants