Hi Myrle, Thanks for your elaborate feedback and corrections.
On Wed, Apr 25, 2018 at 9:10 AM, Myrle Krantz <my...@apache.org> wrote: > Hey Isaac, > > I've looked at your changes in customer and I've found several > problems with them: > > 1.) You've copied all the component tests out of the component test > module into the service module. This means that identical tests will > have to be maintained in two places. Because people aren't very good > at keeping code in sync, you can be certain that these tests will > diverge from each other a little more each time someone makes a > change, and contributors (myself included) won't know which tests are > authoritative. > 2.) You've removed the Apache license header from at least one file. > I added the Apache license header to the build files very recently, as > one step towards making the code releasable. We can't release the > code without the ASF license headers. > Ouch ! My bad. I would have left this commit as a PR for review first. > 3.) I'm assuming the documentation is generated? Do we really want to > check in generated documentation? And where is the process documented > for generating the documentation? > By default, the Spring REST Docs tool outputs documentation in the service's build folder ( which is ignored by git ), so I placed them in service/src/doc so they can be viewed. > > 4.) You added a compile dependency from service to test. The service > should not depend on test. It should be possible to make changes and > add environment variables and etc to the test repository without even > thinking about whether a change might break production code or turn on > test-configurations which weaken security in production code. > > In general dependencies between modules must be 1-way, or else > compilation is difficult or impossible. Component-tests should depend > on service. Service should not depend on component-test. Markus > mentioned this when you asked (2) You might ask, why are component > tests a separate module? There are several reasons: > I was concerned about duplicating the component-tests' tests in the service module and asked about how to avoid the duplication for which I had no feedback.(2) > * We may wish to release the service and api modules and not release > the component-test module. This opens up some possibilities for the > tests that would otherwise be closed off by ASF licensing > requirements. > * Keeping the code seperate helps contributors keep the concerns of > testing and the concern of functionality separate in their thought > processes. Putting testing code in services in other projects has > resulted in poor code or even major security vulnerabilities. It's > important here to think about solutions and their trade-offs carefully > rather than just slapping things together until they "work" and > calling it a day. > * One of the things I hope to accomplish in the future is versioned > component tests to protect against backwards incompatible changes > which can't be detected via signature checking. We can't implement > that plan if the tests become entangled with the source code. > > As a result I think we need to completely rethink the approach to > documentation that you've taken here. Some alternatives we should > consider: > * Move mockMVC into component test and out of service. Why did you > put the documentation and component test code in service in the first > place? > * Create a separate documentation module which imports code from > component-test and service. > * Using Spring Fox instead of MockMVC. When I originally evaluated > REST documentation approaches, it was exactly this mixing of other > concerns into testing that caused me to reject MockMVC in the first > place. Tutorials make good tests but there are many, many good tests > which make terrible documentation. > I'm okay with the approach to documentation being rethought. > > The use of MockMVC came up a few months ago when a potential GSoC > intern suggested it. I had thought, based on your response, that you > understood why it doesn't work well with our architecture. (1) Which > is why I didn't respond to that thread myself. What changed? > > In general, when making architectural changes which change the pattern > of all of the services it's not a good idea to just dump them into all > the services and then ask for feedback. You create a lot of > unnecessary work for yourself and for your code reviewer. A better > approach is to make the change in the fineract-cn-template project > first and then ask for feedback. Better still would be to engage a > discussion on the mailing list before doing a major architectural > change like this. If I had known you were working on this, I could > have shared my thoughts before you started. > I'm sorry I didn't open a discussion here about documenting the APIs. I'm hoping we'll continue that discussion on this thread and fix the mistakes I made. > > Best Regards, > Myrle > > 1.) https://lists.apache.org/thread.html/cc23a47229c262afbb6e4749b7eec3 > 117d084f84144877d59098713d@%3Cdev.fineract.apache.org%3E > 2.) https://lists.apache.org/thread.html/d0fbdb5b21b916cc8dd3c2a5061d3e > 90aa554bb74b63f8deabe69fe6@%3Cdev.fineract.apache.org%3E At Your Service, Isaac Kamga. > > > On Mon, Apr 23, 2018 at 8:42 PM, Isaac Kamga <isaac.ka...@mifos.org> > wrote: > > Hello everyone, > > > > Trust that you're doing great and congratulations to our recently > selected > > GSoC 2018 students. > > > > As you may have observed from Gitbox notifications, I have used the > MockMvc > > flavour of Spring Rest Docs <https://projects.spring.io/spring-restdocs/> > to > > document the APIs for most Fineract CN domain services viz identity, > > office, customer, group, accounting, deposit, payroll and teller, created > > and merged pull requests. There were failing tests in portfolio ( as I > > earlier highlighted last week on the list ) and so snippets weren't > > generated to be used for documentation. > > > > After updating the affected repositories, you can view the documentation > > for each of the services by opening the > > service/src/doc/html5/html5/api-docs.html file. > > > > I happily await your feedback and enhancements. > > > > At Your Service, > > Isaac Kamga. >
