[Issue 154] Refactor PMAccuracy testDataTree method#163
Merged
SergeStinckwich merged 13 commits intomasterfrom Jan 10, 2020
Merged
[Issue 154] Refactor PMAccuracy testDataTree method#163SergeStinckwich merged 13 commits intomasterfrom
SergeStinckwich merged 13 commits intomasterfrom
Conversation
…we can break up the long test method and understand the message better.
…d & identified a possible smell.
…, extracted the expected result to a role-revealing local variable.
…ethod, with a comment identifying a coincidence between expected outcome and a default I discovered in the initialize method.
…now the role it plays in the test yet so gave it a structural name.
SergeStinckwich
approved these changes
Jan 10, 2020
SergeStinckwich
added a commit
that referenced
this pull request
Jan 25, 2020
* [skip CI] * [skip CI] * [skip CI] typos * [skip CI] * Update SMark to 1.0.3 * Update README.md * [Issue 152] Remove Magic From 'initialize' Methods (#153) * [issue-152] Improved code formatting. * [issue-152] Removed some reflection magic so that the code is easier to understand. * [issue-152] Improved code formatting. * Cleanup to load in PharoGs (#150) * Do not use an inst variable name as an argument * Don't use 'p' and 'q' as method temps They are defined as instance variables in the class. If they are not supposed to modify the instance variable, then they should have different names. * Don't begin a block with a dot GemStone isn't happy with empty statements. * Cleanup of Array constructors While legal syntax, GemStone has trouble parsing these. * Don't use instance var name as method argument * Add spacing to clarify when number ends GemStone doesn't like '11do:' as a number since 'd' is a legal exponent. * Spacing to clarify separation of statements * Fix typo with extra space. * [issue-154] Corrected the behaviour of findKey to return 'AllTheRest' when selector has no corresponding initialize message. (#155) * [issue-154] Corrected the behaviour of findKey, making it more testable in the subclass to mitigate risk. * [issue-154] Promoted the local variables in the test to state variable to remove duplication. * [issue-154] Extracted duplicate code to a custom assertion. * [skip CI] * [skip CI] * Create FUNDING.yml * Update FUNDING.yml * Remove curly braces, replaced with Arrays in most cases, should close #159 (#162) * Update dependencies to load from github, should close #160 (#161) * [Issue 154] Refactor PMAccuracy testDataTree method (#163) * [issue-154] Moved the test method to its own test case class so that we can break up the long test method and understand the message better. * [issue-154] Moved some test code to an intention-revealing test method & identified a possible smell. * [issue-154] Moved a block of code to its own intention-revealing test method. * [issue-154] Moved some test code to its own intention-revealing method. * [issue-154] Added a smell comment to help the next developer. * [issue-154] Added more smells to the smell comment. * [issue-154] Moved the test code to an intention-revealing test method, extracted the expected result to a role-revealing local variable. * [issue-154] Extracted test code to its own intention-revealing test method, with a comment identifying a coincidence between expected outcome and a default I discovered in the initialize method. * [issue-154] Added calls to the super class methods as required. * [issue-154] Put the call to the super class method in the right place. * [issue-154] Improved the name of the local variable. We still don't know the role it plays in the test yet so gave it a structural name. * [issue-154] Improved the name of a local variable. * [issue-154] Moved the test code to its own better named test method. * Update .travis.yml * Update appveyor.yml * Fix some of the red tests for Pharo 8 (#164) * Update appveyor.yml * Move from Pharo 7.0 to Pharo 8.0 * Update README.md Co-authored-by: Hemal Varambhia <[email protected]> Co-authored-by: James Foster <[email protected]> Co-authored-by: Julian Maestri <[email protected]>
SergeStinckwich
added a commit
that referenced
this pull request
Jan 27, 2020
* Release PolyMath v1.0.2 (#165) * [skip CI] * [skip CI] * [skip CI] typos * [skip CI] * Update SMark to 1.0.3 * Update README.md * [Issue 152] Remove Magic From 'initialize' Methods (#153) * [issue-152] Improved code formatting. * [issue-152] Removed some reflection magic so that the code is easier to understand. * [issue-152] Improved code formatting. * Cleanup to load in PharoGs (#150) * Do not use an inst variable name as an argument * Don't use 'p' and 'q' as method temps They are defined as instance variables in the class. If they are not supposed to modify the instance variable, then they should have different names. * Don't begin a block with a dot GemStone isn't happy with empty statements. * Cleanup of Array constructors While legal syntax, GemStone has trouble parsing these. * Don't use instance var name as method argument * Add spacing to clarify when number ends GemStone doesn't like '11do:' as a number since 'd' is a legal exponent. * Spacing to clarify separation of statements * Fix typo with extra space. * [issue-154] Corrected the behaviour of findKey to return 'AllTheRest' when selector has no corresponding initialize message. (#155) * [issue-154] Corrected the behaviour of findKey, making it more testable in the subclass to mitigate risk. * [issue-154] Promoted the local variables in the test to state variable to remove duplication. * [issue-154] Extracted duplicate code to a custom assertion. * [skip CI] * [skip CI] * Create FUNDING.yml * Update FUNDING.yml * Remove curly braces, replaced with Arrays in most cases, should close #159 (#162) * Update dependencies to load from github, should close #160 (#161) * [Issue 154] Refactor PMAccuracy testDataTree method (#163) * [issue-154] Moved the test method to its own test case class so that we can break up the long test method and understand the message better. * [issue-154] Moved some test code to an intention-revealing test method & identified a possible smell. * [issue-154] Moved a block of code to its own intention-revealing test method. * [issue-154] Moved some test code to its own intention-revealing method. * [issue-154] Added a smell comment to help the next developer. * [issue-154] Added more smells to the smell comment. * [issue-154] Moved the test code to an intention-revealing test method, extracted the expected result to a role-revealing local variable. * [issue-154] Extracted test code to its own intention-revealing test method, with a comment identifying a coincidence between expected outcome and a default I discovered in the initialize method. * [issue-154] Added calls to the super class methods as required. * [issue-154] Put the call to the super class method in the right place. * [issue-154] Improved the name of the local variable. We still don't know the role it plays in the test yet so gave it a structural name. * [issue-154] Improved the name of a local variable. * [issue-154] Moved the test code to its own better named test method. * Update .travis.yml * Update appveyor.yml * Fix some of the red tests for Pharo 8 (#164) * Update appveyor.yml * Move from Pharo 7.0 to Pharo 8.0 * Update README.md Co-authored-by: Hemal Varambhia <[email protected]> Co-authored-by: James Foster <[email protected]> Co-authored-by: Julian Maestri <[email protected]> * Clean baseline for PM1.0.2 in order to have everything in a same method. Co-authored-by: Hemal Varambhia <[email protected]> Co-authored-by: James Foster <[email protected]> Co-authored-by: Julian Maestri <[email protected]>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Issue #154 is proving a little difficult to fix. Therefore I have begun a targeted refactor of the production and test code to help a future developer understand what the Accuracy package does easily.
This PR breaks up the
testDataTreecode into smaller (hopefully easier to understand) blocks of code.What is interesting here is that the
runmessage is the generator of the dataTree object. It feels that that is what we are really testing.The reason I chose to work on the
testDataTreemessage first is that from my earlier spike, I discovered that part of that method fails when I try to get the Accuracy package covered by coveralls.io in the usual way.