Skip to content

Commit 1a2ab05

Browse files
steinwindesteinwinde
authored
FINERACT-2383: Modernize and correct README page (#5076)
see #5076 https://issues.apache.org/jira/browse/FINERACT-2383 https://lists.apache.org/thread/6nctwb6g6krxxtcxr3q1blx2xb9w8h1s
1 parent 54d7f8b commit 1a2ab05

File tree

2 files changed

+407
-443
lines changed

2 files changed

+407
-443
lines changed

‎CONTRIBUTING.md‎

Lines changed: 233 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,16 +1,240 @@
1+
# Contributing
2+
13
Hello there!
24

3-
First of all, **Thank You** for contributing to Apache Fineract, we're grateful for your interest in our project.
5+
First of all, **Thank You** for contributing to Apache Fineract! We are grateful for your interest in this project.
6+
7+
Please join the [developer mailing list](https://fineract.apache.org/#contribute), if you have not already done so, as this is where discussions about this project take place - say _Hi_ there! Please also have a quick look at the [Code of Conduct](CODE_OF_CONDUCT.md).
8+
9+
The [JIRA Dashboard](https://issues.apache.org/jira/secure/Dashboard.jspa?selectPageId=12335824) shows what's going on. Create a login - it can take a day or two. If you face difficulties, ask on the mailing list.
10+
11+
You don't need to be a committer to provide pull requests, but [Becoming a Committer](https://cwiki.apache.org/confluence/display/FINERACT/Becoming+a+Committer) explains the process of becoming one - just in case...
12+
13+
14+
## Developer How To's
15+
16+
### How to run tests
17+
18+
#### Unit tests
19+
20+
Here's how to run the set of relatively fast and independent Fineract tests:
21+
22+
```bash
23+
./gradlew test -x :twofactor-tests:test -x :oauth2-tests:test -x :integration-tests:test
24+
```
25+
26+
This runs nearly 1,000 tests and completes in a few minutes on decent hardware.
27+
They shouldn't need any special servers/services running.
28+
29+
#### Integration tests
30+
31+
Running tests with external dependencies is a multi-step process with many moving parts.
32+
Sometimes there are arbitrary failures and the prerequisite setup can be daunting.
33+
A full local integration test run (on a developer workstation) covering every possible test using every external service and every supported relational database engine could take an entire day - and that's assuming everything is properly configured and runs as expected.
34+
35+
Right now we depend on GitHub to know if "the build" is passing (it's actually multiple builds).
36+
The authoritative source of truth for what commands/services/tests to run, how, and when are the files in `.github/workflows/`.
37+
Output from runs based on those configuration files appears at <https://github.com/apache/fineract/actions>.
38+
39+
Incorrect default Java-related executables may cause test failures.
40+
To fix this on Debian and Ubuntu systems, run the following:
41+
42+
```bash
43+
export JAVA_HOME=/usr/lib/jvm/zulu21
44+
sudo update-alternatives --set java $JAVA_HOME/bin/java
45+
sudo update-alternatives --set javac $JAVA_HOME/bin/javac
46+
sudo update-alternatives --set javadoc $JAVA_HOME/bin/javadoc
47+
```
48+
49+
This would correct, for example, a [class file version error](https://en.wikipedia.org/wiki/Java_class_file#General_layout).
50+
You might see something like this if a Java 11 executable (class file format version 56) was the system default, but the integration tests were using Java 21 (class file format version 65):
51+
52+
> UnsupportedClassVersionError: com.example.package/ClassName has been compiled by a more recent version of the Java Runtime (class file version 65.0), this version of the Java Runtime only recognizes class file versions up to 55.0
53+
54+
The GitHub builds are run in [short-lived virtual machines](https://docs.github.com/en/actions/using-github-hosted-runners/using-github-hosted-runners), so locally reproducing the same may require additional effort, such as these extra clean-up procedures:
55+
56+
```bash
57+
# Might fix `error: cannot find symbol` or other intermittent failures.
58+
# `doc` here is a placeholder for any task(s) you are trying to run.
59+
# 💚 This is generally very safe to run between builds.
60+
./gradlew --refresh-dependencies doc
61+
62+
# Destroy anything untracked by git.
63+
# ⚠️ This may delete something important, e.g. a finely-tuned IDE configuration.
64+
git clean --force -dx
65+
66+
# Destroy various caches and configs.
67+
# ⚠️ This may delete gibibytes of cached data, making the next build very slow.
68+
rm -rf ~/.gradle ~/.m2 /tmp/cargo*
69+
70+
# Destroy any Java containers left running.
71+
# 💚 This is generally very safe to run between builds.
72+
ps auxwww | grep [c]argo | awk '{ print $2 }' | xargs -r kill
73+
```
74+
75+
Integration test runs such as
76+
```bash
77+
./gradlew --no-daemon --console=plain test -x :twofactor-tests:test \
78+
-x :oauth2-tests:test :fineract-e2e-tests-runner:test -PdbType=postgresql
79+
```
80+
in `.github/workflows/build-postgresql.yml` often take an hour or longer to complete.
81+
If you notice the `:integration-tests:test` task taking significantly less time, say, one minute, gradle may be skipping it.
82+
Look for something like this in the test output:
83+
84+
> Task :integration-tests:test UP-TO-DATE 👀
85+
Custom actions are attached to task ':integration-tests:test'.
86+
Build cache key for task ':integration-tests:test' is 6aeeec3f58bf9703d4c100fbaa657f5c
87+
Skipping task ':integration-tests:test' as it is up-to-date.
88+
Resolve mutations for :integration-tests:cargoStopLocal (Thread[Execution worker Thread 11,5,main]) started.
89+
:integration-tests:cargoStopLocal (Thread[Execution worker Thread 11,5,main]) started.
90+
91+
92+
(This is with the `--info` gradle argument with eyeballs added for emphasis.)
93+
The `--rerun-tasks` gradle argument may help, or you can try destroying `~/.gradle` and other clean-up procedures as indicated above, then re-running tests.
94+
This is useful for repeated test runs (say, for timing) when gradle would otherwise assume a task is "up-to-date" and not re-run it.
95+
96+
See the next section for testing in Eclipse and [here](https://fineract-academy.com) for testing in IntelliJ.
97+
98+
### How to run and debug in Eclipse IDE
99+
100+
It is possible to run Fineract in Eclipse IDE and also to debug Fineract using Eclipse's debugging facilities.
101+
To do this, you need to create the Eclipse project files and import the project into an Eclipse workspace:
102+
103+
1. Create Eclipse project files into the Fineract project by running `./gradlew cleanEclipse eclipse`
104+
2. Import the fineract-provider project into your Eclipse workspace (File->Import->General->Existing Projects into Workspace, choose root directory fineract/fineract-provider)
105+
3. Do a clean build of the project in Eclipse (Project->Clean...)
106+
3. Run / debug Fineract by right clicking on org.apache.fineract.ServerApplication class and choosing Run As / Debug As -> Java Application. All normal Eclipse debugging features (breakpoints, watchpoints etc) should work as expected.
107+
108+
If you change the project settings (dependencies etc) in Gradle, you should redo step 1 and refresh the project in Eclipse.
109+
110+
You can also use Eclipse JUnit support to run tests in Eclipse (Run As->JUnit Test)
111+
112+
Finally, modifying source code in Eclipse automatically triggers hot code replace to a running instance, allowing you to immediately test your changes
113+
114+
How to download Gradle wrapper
115+
---
116+
The file gradle/wrapper/gradle-wrapper.jar binary is checked into this projects Git source repository,
117+
but won't exist in your copy of the Fineract codebase if you downloaded a released source archive from apache.org.
118+
In that case, you need to download it using the commands below:
119+
```bash
120+
wget -P gradle/wrapper https://github.com/apache/fineract/raw/develop/gradle/wrapper/gradle-wrapper.jar
121+
```
122+
or
123+
```bash
124+
curl -L https://github.com/apache/fineract/raw/develop/gradle/wrapper/gradle-wrapper.jar > \
125+
gradle/wrapper/gradle-wrapper.jar
126+
```
127+
128+
### How to run Apache RAT (Release Audit Tool)
129+
130+
1. Extract the archive file to your local directory.
131+
2. Run `./gradlew rat`. A report will be generated under build/reports/rat/rat-report.txt
132+
133+
134+
### How to build documentation
135+
136+
Run the following command:
137+
138+
```bash
139+
./gradlew doc
140+
```
141+
142+
Some dependencies are required (e.g. Ghostscript, Graphviz), see [.github/workflows/build-documentation.yml](https://github.com/apache/fineract/tree/develop/.github/workflows/build-documentation.yml) for hints.
143+
144+
IDEs such as IntelliJ are useful for editing the AsciiDoc source files while providing a live rendered preview.
145+
146+
HTML rendered from the AsciiDoc source files is also available online at [https://fineract.apache.org/docs/current/](https://fineract.apache.org/docs/current/).
147+
148+
149+
## How We Code
150+
151+
### Checkstyle and Spotless
152+
153+
This project enforces its code conventions using [checkstyle.xml](config/checkstyle/checkstyle.xml) through Checkstyle and [fineractdev-formatter.xml](config/fineractdev-formatter.xml) through Spotless. They are configured to run automatically during the normal Gradle build, and fail if there are any violations detected. You can run the following command to automatically fix spotless violations:
154+
```bash
155+
./gradlew spotlessApply
156+
```
157+
Since some checks are present in both Checkstyle and Spotless, the same command can help you fix some of the Checkstyle violations too.
158+
159+
You can also check solely for Spotless violations, but normally don't have to, because regular builds already include this:
160+
```bash
161+
./gradlew spotlessCheck
162+
```
163+
164+
We recommend that you configure your favourite Java IDE to match those conventions. For Eclipse, you can go to
165+
Window > Java > Code Style and import the aforementioned [config/fineractdev-formatter.xml](config/fineractdev-formatter.xml) under formatter section and [config/fineractdev-cleanup.xml](config/fineractdev-cleanup.xml) under cleanup section.
166+
167+
You could also use Checkstyle directly in your IDE, but you don't have to: it may just be more convenient for you. For Eclipse, use https://checkstyle.org/eclipse-cs/ and load our checkstyle.xml into it. For IntelliJ you can use [CheckStyle-IDEA](https://plugins.jetbrains.com/plugin/1065-checkstyle-idea).
168+
169+
170+
### Code Coverage
171+
172+
Changed or added code should ideally have test coverage.
173+
174+
The project uses Jacoco to measure unit tests code coverage. To generate a report run the following command:
175+
```bash
176+
./gradlew clean build jacocoTestReport
177+
```
178+
Generated reports can be found in the build/code-coverage directory.
179+
180+
181+
### Error Handling
182+
183+
* When catching exceptions, either rethrow them, or log them. Either way, include the root cause by using `catch (SomeException e)` and then either `throw AnotherException("..details..", e)` or `LOG.error("...context...", e)`.
184+
* Completely empty catch blocks are VERY suspicious. Are you sure that you want to just "swallow" an exception? Really, 100% totally absolutely sure?? ;-) Such "normal exceptions which just happen sometimes but are actually not really errors" are almost always a bad idea, can be a performance issue, and typically are an indication of another problem - e.g. the use of a wrong API which throws an Exception for an expected condition, when really you would want to use another API that instead returns something empty or optional.
185+
* In tests, you'll typically never catch exceptions, but just propagate them, with `@Test void testXYZ() throws SomeException, AnotherException`..., so that the test fails if the exception happens. Unless you actually really want to test for the occurrence of a problem - in that case, use [JUnit's Assert.assertThrows()](https://github.com/junit-team/junit4/wiki/Exception-testing) (but not `@Test(expected = SomeException.class)`).
186+
* Never catch `NullPointerException` & Co.
187+
188+
### Logging
189+
190+
* We use [SLF4J](http://www.slf4j.org) as our logging API.
191+
* Never, ever, use `System.out` and `System.err` or `printStackTrace()` anywhere, but always `LOG.info()` or `LOG.error()` instead.
192+
* Use placeholder (`LOG.error("Could not... details: {}", something, exception)`) and never String concatenation (`LOG.error("Could not... details: " + something, exception)`)
193+
* Which Log Level is appropriate?
194+
* `LOG.error()` should be used to inform an "operator" running Fineract who supervises error logs of an unexpected condition. This includes technical problems with an external "environment" (e.g. can't reach a database), and situations which are likely bugs which need to be fixed in the code. They do NOT include e.g. validation errors for incoming API requests - that is signaled through the API response - and does (should) not be logged as an error. (Note that there is no _FATAL_ level in SLF4J; a "FATAL" event should just be logged as an _ERROR_.)
195+
* `LOG.warn()` should be using sparingly. Make up your mind if it's an error (above) - or not!
196+
* `LOG.info()` can be used notably for one-time actions taken during start-up. It should typically NOT be used to print out "regular" application usage information. The default logging configuration always outputs the application INFO logs, and in production under load, there's really no point to constantly spew out lots of information from frequently traversed paths in the code about what's going on. (Metrics are a better way.) `LOG.info()` *can* be used freely in tests though.
197+
* `LOG.debug()` can be used anywhere in the code to log things that may be useful during investigations of specific problems. They are not shown in the default logging configuration, but can be enabled for troubleshooting. Developers should typically "turn down" most `LOG.info()` which they used while writing a new feature to "follow along what happens during local testing" to `LOG.debug()` for production before we merge their PRs.
198+
* `LOG.trace()` is not used in Fineract.
199+
200+
## Change Process
201+
202+
### Dependency Upgrades
203+
204+
This project uses a number of 3rd-party libraries. We have set up [Renovate's bot](https://github.com/renovatebot/github-action) to automatically raise Pull Requests for our review when new dependencies are available.
205+
206+
Our `ClasspathHellDuplicatesCheckRuleTest` detects classes that appear in more than 1 JAR. If a version bump in [build.gradle](https://github.com/apache/fineract/blob/develop/build.gradle) causes changes in transitives dependencies, then you may have to add related `exclude` to our [dependencies.gradle](https://github.com/apache/fineract/search?q=dependencies.gradle). Running `./gradlew dependencies` helps to understand what is required.
207+
208+
### Pull Requests
209+
210+
We request that your commit message includes a FINERACT JIRA issue and a one-liner that describes the changes.
211+
Start with an upper case imperative verb (not past form), and a short but concise clear description. (E.g. "FINERACT-821: Add enforced HideUtilityClassConstructor checkstyle").
212+
213+
If your PR is failing to pass our CI build due to a test failure, then:
214+
215+
1. Understand if the failure is due to your PR or an unrelated unstable test.
216+
1. If you suspect it is because of a "flaky" test, and not due to a change in your PR, then please do not simply wait for an active maintainer to come and help you, but instead be a proactive contributor to the project - see next steps. Do understand that we may not review PRs that are not green - it is the contributor's (that's you!) responsibility to get a proposed PR to pass the build, not primarily the maintainers.
217+
1. Search for the name of the failed test on https://issues.apache.org/jira/, e.g. for `AccountingScenarioIntegrationTest` you would find [FINERACT-899](https://issues.apache.org/jira/browse/FINERACT-899).
218+
1. If you happen to read in such bug reports that tests were just recently fixed, or ignored, then rebase your PR to pick up that change.
219+
1. If you find previous comments "proving" that the same test has arbitrarily failed in at least 3 past PRs, then please do yourself raise a small separate new PR proposing to add an `@Disabled // TODO FINERACT-123` to the respective unstable test (e.g. [#774](https://github.com/apache/fineract/pull/774)) with the commit message mentioning said JIRA, as always. (Please do NOT just `@Disabled` any existing tests mixed in as part of your larger PR.)
220+
1. If there is no existing JIRA for the test, then first please evaluate whether the failure couldn't be a (perhaps strange) impact of the change you are proposing after all. If it's not, then please raise a new JIRA to document the suspected Flaky Test, and link it to [FINERACT-850](https://issues.apache.org/jira/browse/FINERACT-850). This will allow the next person coming along hitting the same test failure to easily find it, and eventually propose to ignore the unstable test.
221+
1. Then (only) Close and Reopen your PR, which will cause a new build, to see if it passes.
222+
1. Of course, we very much appreciate you then jumping onto any such bugs and helping us figure out how to fix all ignored tests!
223+
224+
[Pull Request Size Limit](https://cwiki.apache.org/confluence/display/FINERACT/Pull+Request+Size+Limit)
225+
documents that we cannot accept huge "code dump" Pull Requests, with some related suggestions.
226+
227+
Guideline for new Feature commits involving Refactoring: If you are submitting a PR for a new feature,
228+
and it involves refactoring, try to differentiate "new feature code" from "refactored" by placing
229+
them in different commits. This helps to review your code faster.
230+
231+
We have an automated bot which marks pull requests as "stale" after a while, and ultimately automatically closes them.
4232

5-
Please do [join our developer mailing list](https://fineract.apache.org/#contribute), as it's where discussions about this project take place - say _Hi_ there!
6233

7-
Our [JIRA Dashboard](https://issues.apache.org/jira/secure/Dashboard.jspa?selectPageId=12335824) is another good way to see what's going on (do create a Login).
234+
### Merge Strategy
8235

9-
Here are a few additional useful pointers:
236+
This project's committers typically prefer to bring your pull requests in through _Rebase and Merge_ instead of _Create a Merge Commit_. (If you are unfamiliar with GitHub's UI regarding this, note the somewhat hidden little triangle drop-down at the bottom of the PR, visible only to committers, not contributors.) This avoids the "merge commits" which we consider to be somewhat "polluting" the project's commit log history view. We understand this doesn't give an easy automatic reference to the original PR (which GitHub automatically adds to the merge commit message it generates), but we consider this an only very minor inconvenience; it's typically relatively easy to find the original PR even just from the commit message, and JIRA.
10237

11-
* https://github.com/apache/fineract/#pull-requests
12-
* https://github.com/apache/fineract/#checkstyle-and-spotless
13-
* https://github.com/apache/fineract/#logging-guidelines
14-
* https://github.com/apache/fineract/#error-handling-guidelines
238+
We expect most proposed PRs to typically consist of a single commit. Committers may use _Squash and merge_ to combine your commits at merge time, and if they do so, will rewrite your commit message as they see fit.
15239

16-
Note also [our Code of Conduct](CODE_OF_CONDUCT.md).
240+
Neither of these two are hard absolute rules, but mere conventions. Multiple commits in single PRs make sense in certain cases (e.g. branch backports).

0 commit comments

Comments
 (0)