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

Reset the state of the comment plugin on conversion start. #1006

Merged
merged 3 commits into from Apr 6, 2019

Conversation

lddubeau
Copy link
Contributor

@lddubeau lddubeau commented Apr 5, 2019

In the test suite, a single app is reused for all the converter tests. If the plugins do not appropriately reset their state at the start of a conversion, then one test may affect later tests.

CommentPlugin did not reset its hidden field. This caused all converter tests after the comment test to be messed up. In all subsequent tests, the reflections with id 4 and 5 were deleted from project.reflections. This prevented plugins that follow CommentPlugin from acting on reflections 4 and 5. They were still part of the reflection tree however, and were still output, just with incomplete or incorrect values. For instance, kindString was not populated by the GroupPlugin, and fileName was not trimmed by the SourcePlugin.

Commit 3716ba7 incorrectly modified all the tests that were failing due to the incorrect code so that they'd conform to the incorrect output. This PR reverses most of that commit.

Closes #828.

This reverts most of commit 3716ba7.
These were not affected by the revert in the previous commit.
In the test suite, a single app is reused for all the converter tests. If the
plugins do not appropriately reset their state at the start of a conversion,
then one test may affect later tests.
@aciccarello aciccarello merged commit 8086140 into TypeStrong:master Apr 6, 2019
@aciccarello
Copy link
Collaborator

Thanks for finding this. It would be great to eventually rework the testing model to avoid issues like this.

@lddubeau lddubeau deleted the fix/comment-plugin-state branch April 6, 2019 17:50
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.

None yet

2 participants