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

Test OcdFileImport::fillPathCoords #2270

Merged
merged 7 commits into from
Aug 2, 2024

Conversation

dg0yt
Copy link
Member

@dg0yt dg0yt commented Aug 1, 2024

Extended version of the proposed test, #2224 (comment)

dl3sdo and others added 5 commits July 30, 2024 22:47
Move expected data closer to input data.
Format data more readable.
Avoid redundant passing of element counts.
Test path coord processing both for areas and lines.
@dg0yt dg0yt requested a review from dl3sdo August 1, 2024 07:18

void FileFormatTest::ocdPathImportTest_data()
{
#define C(x) ((int)((unsigned int)(x)<<8)) // FIXME: Not the same as in export
Copy link
Member Author

Choose a reason for hiding this comment

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

As long as we don't check the actual x/y values, it doesn't need action.

void FileFormatTest::ocdPathImportTest_data()
{
#define C(x) ((int)((unsigned int)(x)<<8)) // FIXME: Not the same as in export
constexpr auto ocd_flag_gap = 8; // TODO: implement as Ocd::OcdPoint32::FlagGap
Copy link
Member Author

Choose a reason for hiding this comment

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


TestOcdFileImport::OcdImportedPathObject path_object;
ocd_v12_import.fillPathCoords(&path_object, personality == Area, points.size, points.data);
QVERIFY(path_object.getRawCoordinateVector().size() > 0);
Copy link
Member Author

@dg0yt dg0yt Aug 1, 2024

Choose a reason for hiding this comment

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

There was a time when OcdPointsView and FlagsView returned empty views, turning the test into a no-op. I prefer to leave it in.

test/file_format_t.cpp Outdated Show resolved Hide resolved
test/file_format_t.cpp Outdated Show resolved Hide resolved
@dg0yt
Copy link
Member Author

dg0yt commented Aug 2, 2024

Ping @dl3sdo. This has a more direct impact on what can be merged next.

@dl3sdo
Copy link
Member

dl3sdo commented Aug 2, 2024

@dg0yt: Just to understand it right: this unit test is intentionally designed for the 'old' behaviour, i.e. merging #2224 would require an update of this unit test.
Because the expected behaviour of, e.g.,

MapCoord::ClosePoint | MapCoord::HolePoint,
MapCoord::ClosePoint | MapCoord::HolePoint,
MapCoord::ClosePoint | MapCoord::HolePoint,

passes and should not pass after #2224.
I assume that your approach shall allow to track the effects of #2224.

@dl3sdo
Copy link
Member

dl3sdo commented Aug 2, 2024

Kai: this unit test is excellent: it's understandable, easily extensible and it documents the mapping of .ocd coords and attributes to Mapper's coords and attributes. Thank you for the effort you spent on it!

I added another test case but cannot push to your branch, please pick it from ab12494
if you consider it to be useful.

@dg0yt
Copy link
Member Author

dg0yt commented Aug 2, 2024

this unit test is intentionally designed for the 'old' behaviour, i.e. merging #2224 would require an update of this unit test.

Yes, for the moment. It is sort-of a documentation of what is implemented now.
Another commit can add the changes in output that we want to see.
Then another change do the actual modifications to the tested feature.

The test case contains an Ocd::OcdPoint32::FlagDash property which was
yet untested as well as a combination of bezier and straight line
segments.
The area itself contains a hole which itself contains a hole.
@dg0yt dg0yt merged commit 11b068b into OpenOrienteering:master Aug 2, 2024
12 checks passed
@dg0yt dg0yt deleted the test-ocd-coords branch August 2, 2024 20:28
@dg0yt dg0yt restored the test-ocd-coords branch August 3, 2024 05:16
@dg0yt dg0yt deleted the test-ocd-coords branch August 3, 2024 05:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants