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

Add tests for ocs_agent.py #227

Merged
merged 16 commits into from
Sep 22, 2021
Merged

Add tests for ocs_agent.py #227

merged 16 commits into from
Sep 22, 2021

Conversation

BrianJKoopman
Copy link
Member

Description

This PR adds some tests for ocs_agent.py. Perhaps the most interesting part is the introduction of pytest-twisted, for testing methods with @inlineCallbacks, like 'wait'.

This doesn't cover everything in ocs_agent.py, but is a good start. I focused on 'start', 'wait', 'stop', 'abort', and 'status'. Naturally that led to covering register_task and register_process as well.

One thing I wasn't sure how to get to was this exception in 'wait':

except FirstError as e:

There's an attempted test ('test_wait_timeout_w_error'), but it doesn't hit that exception for some reason. Any insight on that would be appreciated.

I also add some very bare test files for modules within ocs that aren't currently imported at all by the tests, namely ocsbow, ocs-cli, and checkdata.

Motivation and Context

Related to #28. Also, I like to see the test coverage number meaningfully increase.

How Has This Been Tested?

I ran the tests, usually just $ python3 -m pytest -m 'not integtest' --cov=ocs --cov-report=html

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • Unless I am preparing a release, I have opened this PR onto the develop branch.

Copy link
Member

@mhasself mhasself left a comment

Choose a reason for hiding this comment

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

My one high-level comment is that I'm slightly uncomfortable testing for the exact text returned (e.g. assert res[1] == 'No task or process called "test_task"') since this text is sort of meant to just be helpful to a human and is not necessarily an unchanging part of the API.

Related, it is probably better to use the enums/constants in base rather for res[0] and res[2]['op_code']. One could argue that we want the enums to never change... but then that should be a separate test :)

ocs/ocs_agent.py Show resolved Hide resolved
mock_agent.register_task('test_task', tfunc, startup=True)

print(mock_agent.startup_ops)
assert mock_agent.startup_ops == [('task', 'test_task', True)]
Copy link
Member

Choose a reason for hiding this comment

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

If you're bothering to test this, you should probably also test the cases of startup=False (should not add to startup_ops!) and startup={'arg1': 12}.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point, added two tests for each register function. This also makes me realize that while startup should be a bool or dict there is nothing stopping one from passing say a list. This hasn't really been run into though, I'm not sure startup is used other than to pass a bool anywhere.

@BrianJKoopman
Copy link
Member Author

My one high-level comment is that I'm slightly uncomfortable testing for the exact text returned (e.g. assert res[1] == 'No task or process called "test_task"') since this text is sort of meant to just be helpful to a human and is not necessarily an unchanging part of the API.

Related, it is probably better to use the enums/constants in base rather for res[0] and res[2]['op_code']. One could argue that we want the enums to never change... but then that should be a separate test :)

Yeah, I totally agree, great feedback as I figure out how to write good tests. I changed these to just check that res[1] is a string, which I think makes sense to do, but let me know if you think I should just drop that too.

Perfect, I also updated the op_code and response_code checks to be against those Enums.

@BrianJKoopman
Copy link
Member Author

Rebased. Will merge when checks finish.

@BrianJKoopman BrianJKoopman merged commit e937839 into develop Sep 22, 2021
@BrianJKoopman BrianJKoopman deleted the test-ocs-agent branch September 22, 2021 19:01
@BrianJKoopman BrianJKoopman mentioned this pull request Nov 29, 2021
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.

None yet

2 participants