unit tests with mock i2c #54

m-mcgowan · 2023-02-10T00:30:13Z

Adds tests over a mock i2c connection.

The tests are factored into common tests for any transport, plus transport-specific tests for mocked i2c and mocked serial.

DRY's up the python commands in github actions by reusing the Makefile.

…long warnings. (120 chars is the norm in many projects and PEP8 allows a maximum of 79.) See https://stackoverflow.com/questions/17319422/how-do-i-set-the-maximum-line-length-in-pycharm#comment58112267_17319775 for discussion.

…hould use the Makefile so they are consistent.)

bsatrom · 2023-02-10T13:59:29Z

test/test_notecard.py

+import unittest
+
 import pytest


Why are you importing unittest when pytest is already present?

unittest supports classes of tests, which I use for reuse of common tests and parameterization of these across different environments.

What's a way we can do this without adding a second unit testing framework?

just a fyi - unittest was already being used for mocks before this PR.

unittest is built into python so we could rework the pytest assertions , which at present are just assertions for raised exceptions. But I wouldn't stop using pytest - it's a great test runner, while unittest deals with expressing tests and creating mocks.

pytest is intended to be used with unittest and supports it out of the box - https://docs.pytest.org/en/7.1.x/how-to/unittest.html#:~:text=pytest%20supports%20running%20Python%20unittest,full%20advantage%20of%20pytest's%20features.

All that said, I've reworked the test classes so they don't have unittest.TestCase as a base class, and pytest is fine with that, so long as the class name starts with Test. The module imports are now as they were and unittest is only used for mocking as before. If we want to move away from unittest for mocks, that's probably best done in a separate PR.

Ah was it? Shows that it's been too long since I've been in this library.

bsatrom · 2023-02-10T14:02:46Z

@m-mcgowan I'm having a hard time seeing what has actually changed in this pr so I am inclined to reject it until it's either simplified or broken into multiple PRs. Rather than taking on a giant refactor of how the tests work, why not add the I2C mocks and tests and we can worry about a refactor later, if at all?

m-mcgowan · 2023-02-10T17:47:57Z

I didn't think it was that big a refactor. All I did was factor out the notecard connection from the tests by moving them in to a base class which calls get_port() to retrieve the port with an expected response, and this is used by two subclasses, one for i2c and one for serial that defines get_port().

It's not presently building on github actions because that's a linux environment and the notecard communications code has the I2C setup hard-coded for to be different for that. It worked on my machine because I'm on OSX - I need to change the logic to also use the same in a test environment so that it is consistent regardless of runtime platform.

When we have HIL testing for this, all we have to do is define another class that uses a real I2C/serial connection running the same tests.

…etup-python#391

m-mcgowan · 2023-02-10T20:26:10Z

mock tests for note-python and I2C

haydenroche5 · 2023-02-10T20:25:27Z

.github/workflows/manual-run.yml

      run: |
        # stop the build if there are Python syntax errors or undefined names
-        flake8 test/ notecard/ examples/ --count --ignore=E722,F401,F403,W503 --show-source --statistics
+        flake8 test/ notecard/ examples/ --count --ignore=E722,F401,F403,W503,E501 --show-source --statistics


Can we add a comment explaining what we're ignoring?

will do. Ideally the github actions should use the makefile so we DRY this up.

haydenroche5 · 2023-02-10T20:34:13Z

test/test_notecard.py

+        assert userAgent['req_port'] is not None

-    port.readline.return_value = "{\"changes\":1}\r\n"
+    def test_debug_mode_on_i2c(self):


The first several lines of this look like what get_i2c_and_port does. Can we use that instead?

Sure. there's definitely more refactoring we could do, I just wanted to do the minimal to get it working.

bsatrom · 2023-02-10T22:04:47Z

I didn't think it was that big a refactor. All I did was factor out the notecard connection from the tests by moving them in to a base class which calls get_port() to retrieve the port with an expected response, and this is used by two subclasses, one for i2c and one for serial that defines get_port().

It's not presently building on github actions because that's a linux environment and the notecard communications code has the I2C setup hard-coded for to be different for that. It worked on my machine because I'm on OSX - I need to change the logic to also use the same in a test environment so that it is consistent regardless of runtime platform.

When we have HIL testing for this, all we have to do is define another class that uses a real I2C/serial connection running the same tests.

My main concern is that this PR is doing two things:

Adding Mock I2C tests, which is great and we should do.
Starting down the path of a refactor to support HiL testing, which we haven't even written a spec or project plan for so I don't see the benefit of starting down that path before we've defined goals, objectives, and set guardrails for that work.

My advice would be to strip out the HiL-pieces of this PR and update with just the I2C pieces.

…ior. Don't use the virtualenv if it doesn't exist.

… and tests. Removes duplication of flake8 flags.

m-mcgowan · 2023-02-10T23:07:57Z

I didn't build this specifically to target HIL, I just wanted to factor out the communications channel, which is orthogonal to most of the tests (i.e. the tests are comms-neutral - they don't care about what channel they are using.) It just happens that with that done, adding HIL later will be easier because we can reuse the same mechanism.

We want a simple way to run all the comms-neutral tests over both mocked serial and i2c, and IMHO this was the simplest way to do it without causing maintenance headaches. The only other way I can think of is to duplicate (copy and paste) all the existing serial tests, and rework them to use i2c, but then we have a lot of duplication of code, which will only become worse when we add more comms channels to the test suite.

bsatrom · 2023-02-11T02:52:10Z

My apologies @m-mcgowan, the combination of an impenetrable diff and mention of HiL caused me to (wrongly) assume that more was going on here than mocking I2C for testing.

m-mcgowan · 2023-02-11T04:27:52Z

No problem! I was going to suggest you view the files side-by-side because then it's much easier to see the diff than what github gives you

m-mcgowan added 3 commits February 9, 2023 16:26

feat: adds mock i2c tests.

9b0ac33

fix: ignore line too long endings in github actions. (Ideally these s…

e1c59f1

…hould use the Makefile so they are consistent.)

bsatrom reviewed Feb 10, 2023

View reviewed changes

m-mcgowan marked this pull request as draft February 10, 2023 18:34

m-mcgowan added 2 commits February 10, 2023 11:53

latest ubuntu (v22) doesn't have python < 3.8, so using v20 actions/s…

516767f

…etup-python#391

add mocking for periphery.I2C

1f50cfd

m-mcgowan marked this pull request as ready for review February 10, 2023 19:56

m-mcgowan requested a review from haydenroche5 February 10, 2023 20:21

haydenroche5 requested changes Feb 10, 2023

View reviewed changes

m-mcgowan added 2 commits February 10, 2023 14:56

chore: separate out the make targets, while preserving existing behav…

105f8fa

…ior. Don't use the virtualenv if it doesn't exist.

chore: use the makefile as the one point for running flake8, docstyle…

69d5ff2

… and tests. Removes duplication of flake8 flags.

m-mcgowan requested a review from haydenroche5 February 10, 2023 23:14

chore: use pytest class declarations, remove the new unittest import.

01ccc47

bsatrom approved these changes Feb 11, 2023

View reviewed changes

haydenroche5 approved these changes Feb 13, 2023

View reviewed changes

m-mcgowan merged commit 239c1b0 into main Feb 13, 2023

m-mcgowan deleted the mdm-mock-i2c branch February 13, 2023 19:56

unit tests with mock i2c #54

unit tests with mock i2c #54

Uh oh!

Conversation

m-mcgowan commented Feb 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

m-mcgowan Feb 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bsatrom commented Feb 10, 2023

Uh oh!

m-mcgowan commented Feb 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

m-mcgowan commented Feb 10, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bsatrom commented Feb 10, 2023

Uh oh!

m-mcgowan commented Feb 10, 2023

Uh oh!

bsatrom commented Feb 11, 2023

Uh oh!

m-mcgowan commented Feb 11, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

m-mcgowan commented Feb 10, 2023 •

edited

Loading

m-mcgowan Feb 10, 2023 •

edited

Loading

m-mcgowan commented Feb 10, 2023 •

edited

Loading