-
Notifications
You must be signed in to change notification settings - Fork 120
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 ComparisonPlace. #644
Add ComparisonPlace. #644
Conversation
…eft and B output on the right.
@jdcove2 why is this label as 'test-only' when this is a new place and feature. Happy to be proven wrong. I have not fully digested this PR yet. |
"Typo" -- I will remove it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor changes requested
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to rebase. Unit tests in this PR were broken by PR #664, and merging this PR in its current state will prevent master from building.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving, but bringing up the concern that this place will introduce invisible places. So it might conflict with the work that was done recently to address invisible places.
I plan to put up another PR to address the "invisible place" issue once this one is merged. Since this place should never be used except locally or on a development system, approving and merging it should not cause any problems. |
This PR adds ComparisonPlace. ComparisonPlace runs the configured two places and gives them identical IBDO's. ComparisonPlace then compares the output of the two places and logs any differences.