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

[pytest] Initial work towards testing tools #39

Merged
merged 24 commits into from
May 20, 2021
Merged

Conversation

gbalke
Copy link
Member

@gbalke gbalke commented May 17, 2021

This adds unit tests to the base level tools. These tests are by no means complete but they establish a baseline. There's also some thrown in adjustments to importing, formatting, and a loop helper to improve loop metrics.

Vaguely on the path of #38. Wanted to make it easier to change stuff while on the go as I'm not always in front of hardware.

gbalke added 15 commits April 7, 2021 09:48
Signed-off-by: Greg Balke <gbalke@berkeley.edu>
Signed-off-by: Greg Balke <gbalke@berkeley.edu>
Signed-off-by: Greg Balke <gbalke@berkeley.edu>
Signed-off-by: Greg Balke <gbalke@berkeley.edu>
Signed-off-by: Greg Balke <gbalke@berkeley.edu>
Signed-off-by: Greg Balke <gbalke@berkeley.edu>
Signed-off-by: Greg Balke <gbalke@berkeley.edu>
Signed-off-by: Greg Balke <gbalke@berkeley.edu>
Signed-off-by: Greg Balke <gbalke@berkeley.edu>
Signed-off-by: Greg Balke <gbalke@berkeley.edu>
Signed-off-by: Greg Balke <gbalke@berkeley.edu>
Signed-off-by: Greg Balke <gbalke@berkeley.edu>
Signed-off-by: Greg Balke <gbalke@berkeley.edu>
Signed-off-by: Greg Balke <gbalke@berkeley.edu>
@gbalke gbalke self-assigned this May 17, 2021
Signed-off-by: Greg Balke <gbalke@berkeley.edu>
Signed-off-by: Greg Balke <gbalke@berkeley.edu>
Signed-off-by: Greg Balke <gbalke@berkeley.edu>
Signed-off-by: Greg Balke <gbalke@berkeley.edu>
Signed-off-by: Greg Balke <gbalke@berkeley.edu>
Copy link
Collaborator

@wuphilipp wuphilipp left a comment

Choose a reason for hiding this comment

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

looks good to me! lots of files looked auto formatted so i did not look very closely at those

tests/tools/unit/test_tools.py Outdated Show resolved Hide resolved
tests/tools/unit/test_tools.py Outdated Show resolved Hide resolved
tests/tools/unit/test_tools.py Show resolved Hide resolved
@gbalke
Copy link
Member Author

gbalke commented May 18, 2021

@wuphilipp I started using an autoformatter (yapf) and plan on introducing it to the python code via another PR. If you think it'd make sense to do this before this PR, to reduce the diff, I can make that work!


print(num_grips)
print(
str(time.time()) + ", " + str(closed_pos) + ", " +
Copy link
Collaborator

Choose a reason for hiding this comment

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

realize this code isn't part of this PR, but this might be easier with an f-string!

f"{time.time()}, {closed_pos}, {opened_pos}, {temperature}, {max_current}\n"

def test_calibrate_encoder(mocker):
"""Ensures read control motor runs when given valid arguments."""
@dataclasses.dataclass
class Args(BaseArgs):
Copy link
Collaborator

Choose a reason for hiding this comment

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

these on-the-fly class declarations feel a little bit hacky to me?

maybe consider something like:

  • Move the Args dataclass to calibrate_encoder.py, which would let you properly hint def action(args): => def action(args: Args):
  • In calibrate_encoder.py: replace the ArgumentParser code in parser_args() with something like datargs; this should be ~1 line
  • Import calibrate_encoder.Args in the test file, and just instantiate it instead of re-defining all of the fields

would cut down some redundant code + make static analysis and autocomplete usable for arguments!

Copy link
Member Author

Choose a reason for hiding this comment

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

Ooooh https://github.com/roee30/datargs#sub-commands. Seems reasonable as it can still address #38.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think a change of this scale (dropping argparse) should be handled when resolving #38. For now, this is a getting off the ground sort of PR. I think the discussions have brought up lots of good points to resolve in follow-up PRs.

@brentyi
Copy link
Collaborator

brentyi commented May 18, 2021

As a note on autoformatting (if you're not particularly attached to yapf yet), maybe also take a look at black?

I think Philipp and I generally both use an isort + black workflow, which is much more aggressive than just yapf. (and probably more common for non-Google employees)

@gbalke
Copy link
Member Author

gbalke commented May 18, 2021

As a note on autoformatting (if you're not particularly attached to yapf yet), maybe also take a look at black?

I think Philipp and I generally both use an isort + black workflow, which is much more aggressive than just yapf. (and probably more common for non-Google employees)

I'd suggest this be handled in a separate issue. I'd be happy to discuss if you'd like to put one together 😄.

Signed-off-by: Greg Balke <gbalke@berkeley.edu>
Signed-off-by: Greg Balke <gbalke@berkeley.edu>
@gbalke gbalke mentioned this pull request May 19, 2021
Signed-off-by: Greg Balke <gbalke@berkeley.edu>
Signed-off-by: Greg Balke <gbalke@berkeley.edu>
@gbalke
Copy link
Member Author

gbalke commented May 20, 2021

Merging this as soon as green CI and then I'll get started on formatting/linting @brentyi.

@gbalke gbalke merged commit 5b0ceff into master May 20, 2021
@gbalke gbalke deleted the script_improvements branch May 20, 2021 03:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants