rm-hull / luma.lcd

Python module to drive PCD8544, HT1621, ST7735, ST7567 and UC1701X-based LCDs
https://luma-lcd.readthedocs.io
MIT License
156 stars 56 forks source link

Ht1621 tests using state machine mocking #72

Closed Gadgetoid closed 5 years ago

Gadgetoid commented 5 years ago

The lack of 100% test coverage on the HT1621 bugged me, so I took a look at the tests and found a completely opaque wall of call() checking that verifies the twiddle of each GPIO pin.

I've implemented the HT1621 as a Python state-machine that decodes the GPIO events back into the original intended data and supplies interfaces to succinctly verify this data against the expected results.

I am unsure how you might feel about complete device state machine mocking, but I've been implementing similar tests across our software repositories. I appreciate the resulting complexity of the test- while it makes them human-readable and maintainable- may cause other issues to creep in such as state-machine edge cases.

This is usually easier with SMBus (as it is in most cases for my repositories) when the communications are well understood and documented.

I did this as much as a mental exercise as I did as a useful contribution, and welcome criticism since I tend to work in a bubble and don't receive it (constructive or otherwise) often.

rm-hull commented 5 years ago

I like it - great stuff 👍.. it certainly does make the tests more readable/maintainable, but I take your point about bugs in the test machinery though.

The 'wall of call' is a result of just synthesizing some simple regression tests off the back of a working implementation, and you'll see this duplicated across all the other driver tests too.

It would be interesting to see if we could adapt this style for other drivers too.

Gadgetoid commented 5 years ago

The 'wall of call' is something I've never come across before- that said my experience of testing is very, very minimal. I understood that it was designed for regression testing, but what wasn't clear to me was how to produce the original call graphs in the first place so that I could reproduce the test setup for- for example- ST7567. I basically made it up as I went along- probably by pasting the untruncated tables of values right out of the tox output (I can't recall now!).

Is there a right way to generate the call tables for regression testing?

In an ideal world we'd have hardware specifications represented in code which we could parse and validate the drivers against- sadly we don't live in an ideal world and everyone's idea of SPI/SMBus/I2C and various things in the middle is different :(

Most SMBus-based devices would fit this pattern quite easily since the very nature of SMBus gives a fairly low surface area of fairly stateless and well understood packets. IE: Generally you write a byte, or you read a byte and that's done via read_byte_data and write_byte_data so you don't have to worry about the protocol level write bit, addressing or any bit twiddling at all.

This isn't always the case though. The AS7262 Spectral Sensor (not an LCD, but for example) doesn't implement SMBus in a sane way, but uses only 3 registers to denote read, read and status. Here's how that has to be implemented to facilitate testing: https://github.com/pimoroni/as7262-python/blob/master/library/tests/tools.py

It's ugly. But not quite so bad as the entire state machine that HT1621 requires.

Here's some slightly more down to earth SMBus mocking using a fairly typical pattern: https://github.com/pimoroni/bh1745-python/blob/master/library/tests/tools.py

The parent class MockSMBus simply defines a list of bytes to take the place of the SMBus device registers and these can be populated with things like Chip ID and Manufacturer ID that a library expects to be able to read from the hardware during instantiation.

Most other SPI devices should be pretty straight forward, too, since they rely almost universally on 8-bit words and not the spectacularly weird 3, 4, 6 and 8bit values that the HT1621 understands. Most of the complexity in the HT1621 testing state machine is just dealing with these weird values and splitting a continuous stream of bits into 8bit bytes without any clear denotation.

I must add- thank you for the luma libraries and, in particular, for your test setup which I learned a great deal from and which helped me roll out testing across many of our libraries.

thijstriemstra commented 5 years ago

I must add- thank you for the luma libraries and, in particular, for your test setup which I learned a great deal from and which helped me roll out testing across many of our libraries.

that's great to hear :+1:

Gadgetoid commented 5 years ago

@thijstriemstra the form I always use for docstrings is:

"""Brief outline.

Further details.

:param x: x does something
:param y: y does something
:param z: z does something

"""

Which are then understood by tools like Sphinx and converted to: http://docs.pimoroni.com/blinkt/

What's the pattern here? Is Luma standardised on just one-line outlines?

thijstriemstra commented 5 years ago
"""
Like this.
"""
Gadgetoid commented 5 years ago

Aha! Gotcha.

(I was reading the question as "is it valid to put doc strings on 3 lines" and not "hey Gadgetoid dummy, have you even read the other code in this repository? Please format the docstrings to match convention!" will fix!)

Gadgetoid commented 5 years ago

Done!