Cleanup & comment .chalk file writer tests #570
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We test the program writer by parsing a program, writing it, and reparsing the output. We compare the original Program against the writer's output using Program's Eq impl. This Eq impl relies on item ids, which are dependent on the order the items appear in the program source. Previously, we would output one type of item at a time. That meant the original ordering would not be preserved, unless the program was written with the items grouped, in the correct ordering. This is a weird and frustrating quirk that makes the tests less readable. We have updated `write_program` to preserve the ordering. The items are sorted by their RawId, which will result in the original ordering. Co-authored-by: David Ross <daboross@daboross.net>
Refactored the syntax writer tests so there is only one reparse_test call per test function. This involved removing reduntant tests, and creating new test functions. We also added comments for all of the test to describe what particular functionality the test was targeted at. This is useful when a test's purpose is obscured by the substantial boilerplate that test requires. Co-authored-by: David Ross <daboross@daboross.net>
daboross
commented
Jul 11, 2020
| @@ -25,7 +25,7 @@ pub use self::state::*; | |||
|
|
|||
| use self::utils::as_display; | |||
|
|
|||
| pub fn write_item<F, I, T>(f: &mut F, ws: &WriterState<'_, I>, v: &T) -> Result | |||
| fn write_item<F, I, T>(f: &mut F, ws: &WriterState<'_, I>, v: &T) -> Result | |||
There was a problem hiding this comment.
This is technically unrelated, but fairly insubstantial.
There was a problem hiding this comment.
Well, as long as write_items is still public and usable, this should be fine
There was a problem hiding this comment.
👍! I think it is a positive change, just that it probably should have ended up part of #558 rather than this PR.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This addresses another issue raised in the final review of #430 - mainly, that none of the tests explained at all what they're doing, and multiple tests were often merged into one test function.
We've made sure that each test makes sense, has comments, and only tests a single item. This involved removing reduntant tests, and creating new test functions. The comments are useful when a test's purpose is obscured by the substantial boilerplate it requires.