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

Create a test file for orchestrator.py #23

Merged
merged 5 commits into from Jan 15, 2020
Merged

Create a test file for orchestrator.py #23

merged 5 commits into from Jan 15, 2020

Conversation

@maanika
Copy link
Member

maanika commented Jan 13, 2020

Description

This pull request will create a test file for orchestrator.py

Enter 'pytest' in the terminal to run the tests

@maanika maanika changed the title if applied, this commit will create a test file for orchestrator.py Create a test file for orchestrator.py Jan 13, 2020
@angustrau

This comment has been minimized.

Copy link
Member

angustrau commented Jan 13, 2020

Could you add some documentation on how to run the tests?

Copy link
Member

hallgchris left a comment

A few comments. To elaborate on what Angus said, some quick instructions on how to run the tests in the readme.md would be good, so that people don't have to refer to this PR to see how to run tests.

import orchestrator

class TestOrchestrator:
"""Orchestrator test class"""

This comment has been minimized.

Copy link
@hallgchris

hallgchris Jan 13, 2020

Member

To me a lot of this documentation seems unnecessary, as the class/method names already tell us what the comments are saying. In saying this, I've realised that @khanguslee has made similar documentation in the BOOST tests, so maybe he has a reason for it...?

This comment has been minimized.

Copy link
@hallgchris

hallgchris Jan 13, 2020

Member

Also, just for neatness, chuck a space at the start and end of the """ comments, and before the # comments

This comment has been minimized.

Copy link
@khanguslee

khanguslee Jan 13, 2020

Member

I did it in BOOST to make pylint happy. Really not sure what they want inside them without being too verbose.

This comment has been minimized.

Copy link
@hallgchris

hallgchris Jan 14, 2020

Member

Fair enough!Maybe as an alternative, we should consider disabling the check for docstrings in our test modules. Haven't tested, but the following line at the top of the file should work:

# pylint: disable=missing-docstring
CameraOverlay/tests/orchestrator_test.py Outdated Show resolved Hide resolved
CameraOverlay/tests/orchestrator_test.py Outdated Show resolved Hide resolved
CameraOverlay/tests/orchestrator_test.py Show resolved Hide resolved
Copy link
Member

angustrau left a comment

You should add pytest to the requirements.txt as it isn't installed by default

maanika added 2 commits Jan 14, 2020
…t.py, requirements.txt and ReadMe.txt
@maanika maanika requested review from hallgchris and angustrau Jan 14, 2020
requirements.txt Outdated Show resolved Hide resolved
@maanika maanika requested a review from angustrau Jan 14, 2020
Copy link
Member

angustrau left a comment

Good job 👍

Copy link
Member

hallgchris left a comment

Two things.

By the way, when naming commits and PRs, the "if applied, this commit will..." thing doesn't need to be in the actual title, it just needs to make sense when put before the actual name of your commit/PR. e.g. Add required changes to orchestrator_tespy, requirements.txt and ReadMe.txt is a valid commit name because it makes sense in the sentence If applied, this commit will add required changes to orchestrator_tespy, requirements.txt and ReadMe.txt.

CameraOverlay/tests/orchestrator_test.py Outdated Show resolved Hide resolved
Documentation/ReadMe.txt Outdated Show resolved Hide resolved
Copy link
Member

harsilspatel left a comment

pretty much what Chris said, those are minor things but will definitely be helpful in the long run! other than that great work @maanika! :D

@maanika maanika dismissed stale reviews from harsilspatel and angustrau via 477146a Jan 15, 2020
@maanika maanika requested a review from hallgchris Jan 15, 2020
Copy link
Member

hallgchris left a comment

Looks good! Go ahead and merge :)

@maanika maanika merged commit ac2e01c into master Jan 15, 2020
1 check failed
1 check failed
Codacy/PR Quality Review Not up to standards. This pull request quality could be better.
Details
@maanika maanika deleted the maanika/pythontests branch Jan 15, 2020
@khanguslee

This comment has been minimized.

Copy link
Member

khanguslee commented Jan 15, 2020

Two things.

By the way, when naming commits and PRs, the "if applied, this commit will..." thing doesn't need to be in the actual title, it just needs to make sense when put before the actual name of your commit/PR. e.g. Add required changes to orchestrator_tespy, requirements.txt and ReadMe.txt is a valid commit name because it makes sense in the sentence If applied, this commit will add required changes to orchestrator_tespy, requirements.txt and ReadMe.txt.

Additionally, a commit should actually change one thing!. If you sentence ever has an 'and' in it. You definitely missed a commit!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.