Skip to content

Wrote down test suiteversion 1 - unit tests and end-to-end tests for the original monkey-patch core file of Hallmark#11

Closed
rohinsant wants to merge 2 commits intomainfrom
test_suite
Closed

Wrote down test suiteversion 1 - unit tests and end-to-end tests for the original monkey-patch core file of Hallmark#11
rohinsant wants to merge 2 commits intomainfrom
test_suite

Conversation

@rohinsant
Copy link
Contributor

Summary

Adding the test suit including the unit test and end-to-end test for the monkey-patched version of hallmark. Issue #10

Details

The end-to-end test checks the filter functionality and the debug statements of ParaFrame while the unit tests check the structural features of the ParaFrame.

Testing

We ran pytests on the two files and they passed successfully.

Follow-on

Updating the core file to the subclassed version of ParaFrame and modifying the test suite accordingly. Split debug unit test into its own end-to-end test.

@rohinsant rohinsant requested a review from hfoote November 11, 2025 21:27
Copy link
Member

@hfoote hfoote left a comment

Choose a reason for hiding this comment

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

Great start! There's two quick things I'd like to clean up before we merge this:

  • Please make sure the create_temp_data fixture is only defined in one place.
  • Please make sure test_type_of_ParaFrame is consistent with the currently expected behavior, rather than commenting it out.

I left more detailed notes on these in the comments. Other than that, this looks good!

from hallmark import ParaFrame

@pytest.fixture
def create_temp_data(tmp_path):
Copy link
Member

Choose a reason for hiding this comment

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

This fixture shouldn't be defined the same way in two separate files. It's recommended to define fixtures that are shared across test files in a conftest.py file, see the pytest docs.

fmt = str(create_temp_data / "a_{a:d}/b_{b:d}.txt")
return ParaFrame(fmt, debug = True)

# def test_type_of_ParaFrame(create_ParaFrame):
Copy link
Member

Choose a reason for hiding this comment

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

I assume this is commented out because ParaFrame is still a DataFrame in the current implementation? If I'm wrong, let me know. For now, it's probably a good idea to test that a ParaFrame is an instance of a DataFrame since that's the currently expected behavior.

Either way, it's better practice to skip a test (with a skip mark that notes the reason the test is being skipped) than to comment one out.

… added a test to check the ParaFrame type of the monkey-patched core file
@rohinsant rohinsant closed this Nov 24, 2025
@rohinsant rohinsant deleted the test_suite branch January 21, 2026 17:23
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.

3 participants