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. 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? 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: * 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. 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. Best Regards, Myrle 1.) https://lists.apache.org/thread.html/cc23a47229c262afbb6e4749b7eec3117d084f84144877d59098713d@%3Cdev.fineract.apache.org%3E 2.) https://lists.apache.org/thread.html/d0fbdb5b21b916cc8dd3c2a5061d3e90aa554bb74b63f8deabe69fe6@%3Cdev.fineract.apache.org%3E On Mon, Apr 23, 2018 at 8:42 PM, Isaac Kamga <[email protected]> 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.
