- Notifications
You must be signed in to change notification settings - Fork244
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
(FAF149) Module dependency computation enhancement#700
base:support/3.2
Are you sure you want to change the base?
Conversation
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.
seems ok for me
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.
As this method is hard to test manually, changing it means covering it completely. I have added a few suggestions, but it will require an additional review (interactive) to make sure 100% of the rules are covered.
| \ModuleDiscovery::SortModulesByCountOfDepencenciesDescending($aOngoingDependencies); | ||
| $this->assertEquals($aExpectedKeys, array_keys($aOngoingDependencies)); |
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.
I'd rather have $aOngoingDependencies and $aExpectedKeys shown explicitely as PHP arrays, thus documenting the input and output of the tested method. We could even inline intermediate variables like this:
$this->assertEquals( [3,2,1],array_keys(\ModuleDiscovery::SortModulesByCountOfDepencenciesDescending( [1 => [2],2 => [3],3 => [], ] )););
Moreover, I wonder if this is meaningful to test this internal method. This could be the case to explore corner cases. If so, then other cases should be added. Otherwise, the tests implemented after that one should be sufficient to cover this method, and the test ofSortModulesByCountOfDepencenciesDescending should be removed.
| $this->assertEquals($aExpectedKeys, array_keys($aOngoingDependencies)); | ||
| } | ||
| public function testOrderModulesByDependencies_CheckMissingDependenciesAreCorrectlyOrderedInTheException() |
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.
Function naming: "correctly" could be clarified. Alternatively, this could be clarified in the assertion message.
$this->expectExceptionMessage($sExpectedMessage, 'Modules with fewest count of missing dependencies should reported first');
setup/modulediscovery.class.inc.php Outdated
| { | ||
| $aModule = $aModules[$sId]; | ||
| $aDepsWithIcons = []; | ||
| $aDeps=$aDependencies[$sId]; |
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.
Coding convention
| \ModuleDiscovery::OrderModulesByDependencies($aModules, true); | ||
| } | ||
| public function testOrderModulesByDependencies_ResolveOk() |
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.
Coverage for successful cases could be completed by added the following [corner] cases:
- No dependency at all
- Several dependencies on a given module
| ]; | ||
| $this->assertEquals($aExpected, array_keys($aResult)); | ||
| } | ||
| } |
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.
Coverage for failing cases could be improved by testing:
- Circular reference
- bAbortOnMissingDep = false. What is the expected behavior then?
This PR brings few optimizations in dependency computation. It also enhances iTop feedback when there are module dependency issues: modules with less missing dependencies are printed first.
Usually modules dependencie issues are due to one missing module. it is hard to find it out when unresolved cascading dependencies are displayed in a "random" order.
Example (BEFORE)
Example (with PR enhancement): Now iTop XXX will come first.