JCgH4164838Gh792C124B5 commented on PR #573: URL: https://github.com/apache/struts/pull/573#issuecomment-1221355624
Hi @yasserzamani. Thanks for the review, and more background on the DI and alias behaviour. 😄 > At bottom this LGTM and will merge. Just one thing. A few tests which are from past and not related, are deleted, I think by mistake, right? If so, then could you revert them. thanks! The `testXXXAlternateConstructor()` tests were intentionally removed with the recent set of changes, as they no longer made sense to me once the changes you made to the parameterized `OgnlUtil` constructor were applied to this PR. Those deleted tests were originally code coverage "duplicate" tests of existing tests to cover `OgnlUtil(null, null)` calls, but now using null parameters is invalid and will result in `IllegalArgumentException` , so those tests no longer seemed to make sense. The general test for `OgnlUtil` with null parameters should now be covered by `testDefaultOgnlUtilAlternateConstructorArguments()` , and there are still the `testXXXAlternateConstructorPopulated()` tests covering non-null parameters. Please let me know if the above makes sense, and if you agree the test changes are valid ? If not, I can restore the deleted tests and make changes to pattern them like `testDefaultOgnlUtilAlternateConstructorArguments()`. Please let me know what you think. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
