Skip to content

Input class unit tests#145

Merged
nakul680 merged 24 commits into
faccts:mainfrom
nakul680:unittests
Dec 1, 2025
Merged

Input class unit tests#145
nakul680 merged 24 commits into
faccts:mainfrom
nakul680:unittests

Conversation

@nakul680

Copy link
Copy Markdown
Collaborator
  • added unit tests for simple keyword functions
  • added eq method for SimpleKeyword class to compare simple keywords based on the keyword

@nakul680 nakul680 requested a review from haneug September 24, 2025 10:07
@nakul680 nakul680 self-assigned this Sep 24, 2025
@nakul680 nakul680 added the side input Concerning writing ORCA input label Sep 24, 2025
@haneug haneug self-assigned this Sep 24, 2025
@haneug haneug linked an issue Sep 24, 2025 that may be closed by this pull request
@haneug haneug added the testsuite Concerning the testsuite of OPI label Sep 24, 2025
@haneug haneug added this to the 2.0.0 milestone Sep 24, 2025

@haneug haneug left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

From what I see it would be totally reasonable to have all the input tests in one test file (~100-200 lines of test). Also please add a marker unit for the unit tests so that we can run them separately with pytest -m unit in the CI

@nakul680 nakul680 requested a review from haneug October 6, 2025 09:36

@haneug haneug left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is already a good start 👍
You should consider the following things moving forward:

  • put the unit tests in their own sub folder so one can easily run only the unit tests, e.g., tests/unit/
  • please make use of @pytest.mark.parametrize for similar tests that differ only in input data to reduce the number of tests and to get better control and cleaner overview over them. I commented one example and I hope that is already enough to get the gist.
  • please try to make the unit tests more specific. In the integration tests (example tests for ORCA) we rely on ORCA and therefore we did not want to test too thoroughly but here we should be as specific as possible, e.g., instead of len(blocks) == 2, check set(blocks.keys()) == {BlockA, BlockB}
  • every test should have a short docstring on what is tested.

Comment thread tests/test_input_simple_keywords.py Outdated
Comment thread tests/test_input_simple_keywords.py Outdated
Comment thread tests/test_input_simple_keywords.py Outdated
Comment thread tests/unit/test_input_blocks.py
Comment thread tests/test_input_block.py Outdated
@nakul680 nakul680 requested a review from haneug October 8, 2025 13:34
@nakul680

nakul680 commented Oct 8, 2025

Copy link
Copy Markdown
Collaborator Author

I added a eq() function for the ArbitraryString class for the purposes of testing , I'm not sure if this was the right away to go about it.

haneug
haneug previously requested changes Oct 9, 2025

@haneug haneug left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I like the addition of test_arbitrary_strings and the way you used parametrize. I just have a few minor comments and suggestions you could try:

  • Please rename test_arbitray_strings.py to test_input_arbitrary_strings.py for consistency.
  • In test_input_simple_keywords.py, you (almost) repeat the same parametrize three times. You could consider defining a single fixture with params instead and reusing it.
  • If possible, I’d avoid hard-coding simple_keywords directly in tests. You should try to use fixtures or parameters where it is possible (I made some comments).
  • For the docstrings, I’d avoid describing parameter-specific behavior (see my comments). As a general suggestion: try to keep them to short one-liners that describe the behavior under test, instead of repeating the function signature. I really liked how you did this in test_arbitrary_strings.py.

One last thing: please try to avoid force-pushing (if possible) and merge instead. A clean history make the review a bit easier :)

Comment thread tests/unit/test_input_simple_keywords.py Outdated
Comment thread tests/unit/test_input_simple_keywords.py Outdated
Comment thread tests/unit/test_input_simple_keywords.py Outdated
Comment thread tests/unit/test_input_simple_keywords.py Outdated
Comment thread tests/unit/test_input_simple_keywords.py Outdated
Comment thread tests/unit/test_input_blocks.py Outdated
Comment thread tests/unit/test_input_blocks.py Outdated
Comment thread tests/unit/test_input_blocks.py Outdated
Comment thread tests/unit/test_arbitrary_strings.py Outdated
Comment thread tests/unit/test_arbitrary_strings.py Outdated
@nakul680 nakul680 requested a review from timmyte November 5, 2025 09:33
Comment thread src/opi/input/arbitrary_string.py
Comment thread tests/unit/test_arbitrary_strings.py Outdated
Comment thread tests/unit/test_arbitrary_strings.py Outdated
Comment thread tests/unit/test_arbitrary_strings.py Outdated
Comment thread tests/unit/test_arbitrary_strings.py Outdated
Comment thread tests/unit/test_input_blocks.py Outdated
Comment thread tests/unit/test_input_blocks.py Outdated
Comment thread tests/unit/test_input_simple_keywords.py Outdated
Comment thread tests/unit/test_input_simple_keywords.py Outdated
Comment thread tests/unit/test_input_simple_keywords.py Outdated
@nakul680 nakul680 requested a review from timmyte November 12, 2025 14:10

@timmyte timmyte left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please remove the blanks in the documentation of strict = True.

Otherwise, thanks for adding the requested fixtures.
And I hope you can see the advantage of not xxx over len(xxx) = 0.

Comment thread tests/unit/test_input_arbitrary_strings.py
Comment thread tests/unit/test_input_blocks.py Outdated
Comment thread tests/unit/test_input_arbitrary_strings.py Outdated
Comment thread tests/unit/test_input_arbitrary_strings.py Outdated
Comment thread tests/unit/test_input_blocks.py Outdated
Comment thread tests/unit/test_input_blocks.py Outdated
Comment thread tests/unit/test_input_simple_keywords.py Outdated
@nakul680 nakul680 requested a review from timmyte November 13, 2025 12:12

@timmyte timmyte left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Great. Nice

@nakul680
You can proceed to merge the PR, but please fix Hagen's last open requests. Just click on "Resolve conversation" when they are done.
Once done, please squash all your changes and to don't forget to add a closes #71 tag, to close the corresponding issue.

@nakul680 nakul680 changed the title WIP: Input class unit tests Input class unit tests Nov 19, 2025
@timmyte timmyte dismissed haneug’s stale review December 1, 2025 12:14

Other reviewer approved

@timmyte timmyte requested a review from a team as a code owner December 1, 2025 12:15
@nakul680 nakul680 merged commit b05c49f into faccts:main Dec 1, 2025
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

side input Concerning writing ORCA input testsuite Concerning the testsuite of OPI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add unit tests

3 participants