Hello Myrle, I've done some tinkering and Spring REST Docs can indeed get the snippets generated for documentation in the component-test module. At least, that ensures that there's no need for an architectural change and the service module doesn't depend on the component-test module as Markus advised.
I think there are still other concerns which you highlighted; 1. Checking in generated documentation : Should we leave them in component-test's build folder (which is ignored by git) and rather document the process of generating the snippets ? 2. Creating a documentation module : Do we still need to try this out ? This module will import the service and component-test modules. 3. MockMvc vs Spring Fox: Were you concerned about the security of MockMvc ? I'd like to have this discussed here so that we don't proceed with approaches to solving the problem which aren't worthwhile. I had earlier assumed that a mention on the Quarter 1 board report about my work on API documentation would have sufficed. If you're okay with the API documentation occurring in the component-test module, it may be okay for me to revert the commits I made to the repositories and submit a Pull request which you'll review and merge. At Your Service, Isaac Kamga. On Wed, Apr 25, 2018 at 11:53 AM, Isaac Kamga <[email protected]> wrote: > Hi Myrle, > > Thanks for your elaborate feedback and corrections. > > On Wed, Apr 25, 2018 at 9:10 AM, Myrle Krantz <[email protected]> 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/cc23a47229c262afbb6e474 >> 9b7eec3117d084f84144877d59098713d@%3Cdev.fineract.apache.org%3E >> 2.) https://lists.apache.org/thread.html/d0fbdb5b21b916cc8dd3c2a >> 5061d3e90aa554bb74b63f8deabe69fe6@%3Cdev.fineract.apache.org%3E > > > At Your Service, > Isaac Kamga. > >> >> >> 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/sp >> ring-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. >> > >
