Conversation
| # pylint: disable=abstract-method | ||
| @dataclasses.dataclass | ||
| class RawImage(Image): | ||
| """For testing functions implemented in the base Image class.""" |
There was a problem hiding this comment.
This docstring isn't true anymore, and I'd rather focus on what it does rather than what it's used for
| with db.batch() as batch: | ||
| batch.set_recomp(4, name="Hello") | ||
| batch.match(4, 4) | ||
|
|
||
| orig = RawImage.from_memory(b"\x04\x00\x00\x00") | ||
| recomp = RawImage.from_memory(b"\x04\x00\x00\x00") | ||
| comparator = VariableComparator(db, types, orig, recomp) |
There was a problem hiding this comment.
The test could be even more expressive by having different addresses for the addresses the pointers point to. Applies to the test below as well.
| """For testing functions implemented in the base Image class.""" | ||
|
|
||
| # Total size of the image. | ||
| # If it is more than the size of physical data, the remainder is uninitialized (all null). |
There was a problem hiding this comment.
Read the comment below first, it may make this comment obsolete.
I'd move/copy that into a new docstring for from_memory, and emphasise that size may be larger than data.
tests/raw_image.py
Outdated
| if size is None: | ||
| maxsize = len(data) | ||
| else: | ||
| maxsize = max(size, len(data)) |
There was a problem hiding this comment.
What do you think about replacing size by additional_uninitialized_bytes, which must be >= 0, default 0. This may also make the tests below more self-explanatory.
In case you want to keep size: I'd rather raise an error here if size < len(data). This is most likely unintentional and the test author may be suprised by this behaviour.
There was a problem hiding this comment.
I like it, thanks! I only had to change a couple things around in test_image_raw.py to get it working.
What do you think of bss as the parameter name? We're already using that abbreviation for uninitialized data in variables.py. Since it's required to be a kwarg I think it should be short if possible.
| assert c.compared[1].match is False | ||
|
|
||
|
|
||
| def test_compare_complex_with_padding(db: EntityDb): |
There was a problem hiding this comment.
Nice, I didn't even know this was supported already!
Should we also test padding in the middle?
| assert c.result == CompareResult.DIFF | ||
|
|
||
|
|
||
| @pytest.mark.xfail(reason="GH #305") |
There was a problem hiding this comment.
I like this pattern of linking xfails to GitHub issues. Maybe the issue should reference its xfails tests as well, so we don't forget to re-enable them before closing the ticket.
| assert c.result == CompareResult.MATCH | ||
|
|
||
|
|
||
| def test_compare_complex_raw_missing_key(db: EntityDb, types: CvdumpTypesParser): |
There was a problem hiding this comment.
should this have a mismatch test as well?
2 participants
Closes #303.
Moves the comparison logic for variables out of
tools/datacmp.pyand into a new variable comparator class, similar to the existing one for functions. This allows us to drag in specific dependencies from the mainCompareobject so they can be mocked in tests. Theis_pointer_matchfunction incore.pywas only used bydatacmp, so I moved it here.MockTypesDballows us to define complex types without reading cvdump text, as we've done in other tests.I also set up a file for the existing
RawImageclass because it is now used in two test modules. This allows us to define the initialized and uninitialized data for an image without having to create a complex mock on theread()function.My main focus was testing primitive type variables in different scenarios. Since
get_scalars_gapless()function reduces the complex type to a list of these scalar values, I felt that this wasn't that different from comparing a single value.