Nothing Special   »   [go: up one dir, main page]

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

Add unit tests for serialization in ToolEnum. #3462

Merged

Conversation

createyourpersonalaccount
Copy link
Contributor

Checks the following invariant:

fromString(toString(x)) == x

for all enum values of ToolSize and ToolType.

This check is motivated by a bug discovered, where this invariant failed
to hold, because one string was "playObject", while another was
"PlayObject".

We also modify continuous-integration.yml, since macOS unit tests are
not automatically discovered, to include our unit test binary
test-toolEnums.

Checks the following invariant:

    fromString(toString(x)) == x

for all enum values of ToolSize and ToolType.

This check is motivated by a bug discovered, where this invariant failed
to hold, because one string was "playObject", while another was
"PlayObject".

We also modify continuous-integration.yml, since macOS unit tests are
not automatically discovered, to include our unit test binary
test-toolEnums.
@createyourpersonalaccount
Copy link
Contributor Author

This depends on #3467

@LittleHuba
Copy link
Member

Please rebase onto release-1.1

@createyourpersonalaccount createyourpersonalaccount changed the base branch from master to release-1.1 October 13, 2021 20:58
@createyourpersonalaccount
Copy link
Contributor Author

Done.

Copy link
Member
@LittleHuba LittleHuba left a comment

Choose a reason for hiding this comment

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

LGTM!
@xournalpp/core Merge in 24 hours if there are no objections.

@LittleHuba LittleHuba added the merge proposed Merge was proposed by maintainer label Oct 21, 2021
Copy link
Contributor
@idotobi idotobi 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. Great idea for a test 👍

@bhennion
Copy link
Collaborator

Should we merge this then?

@LittleHuba LittleHuba merged commit 7bf1c91 into xournalpp:release-1.1 Oct 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge proposed Merge was proposed by maintainer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants