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 more tests for format=original. #13584

Merged
merged 2 commits into from Nov 5, 2018
Merged

Add more tests for format=original. #13584

merged 2 commits into from Nov 5, 2018

Conversation

diosmosis
Copy link
Member

No description provided.

@diosmosis diosmosis added Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. c: Tests & QA For issues related to automated tests or making it easier to QA & test issues. labels Oct 11, 2018
@diosmosis diosmosis added this to the 3.7.0 milestone Oct 11, 2018
@diosmosis diosmosis added Needs Review PRs that need a code review and removed Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. labels Oct 11, 2018
Copy link
Member

@sgiehl sgiehl left a comment

Choose a reason for hiding this comment

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

@diosmosis any reason for adding those tests? Imho it's very hard to validate if the content of each test file is correct. If it's only for proving the format original works, maybe there's a better way to check without comparing any file contents?

@diosmosis
Copy link
Member Author

The original format is going to be used for another plugin that will unserialize after getting serialized data back over a network. Which means it can be easy to add new data to the result of an API method that cannot be unserialized.

I suppose an alternative here is to just serialize & then unserialize it. Will look into that.

@diosmosis
Copy link
Member Author

Updated to just unserialize the response and check it unserialized.

Copy link
Member

@sgiehl sgiehl left a comment

Choose a reason for hiding this comment

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

Looks much better now 👌

@sgiehl sgiehl merged commit e5fcade into 3.x-dev Nov 5, 2018
@sgiehl sgiehl deleted the format-original-tests branch November 5, 2018 08:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: Tests & QA For issues related to automated tests or making it easier to QA & test issues. Needs Review PRs that need a code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants