Page Comparison

...

2024 Observations

• MockServer has become unmaintainable in mod-orders (2980 lines with a lot of interdependant pieces related to testing different functionalities, any change can break any number of tests)

• Other test classes like PurchaseOrdersApiTest (4355 lines) or InvoicesApiTest (4214 lines) have become too large and are also hard to maintain.

• Unit tests are slow and can fail randomly; this slows down development considerably.

• Testing the API with real REST calls is not necessary for unit tests, is slow and is better done with integration tests when needed (using Karate).

• There is no longer a structural difference between "helper" classes and services (only services used to be created with ApplicationConfig, and services used to be small in scope), and the helper classes tend to have too many dependencies (see for instance PurchaseOrderHelper in mod-orders or InvoiceHelper in mod-invoice).

Guidelines

Notes about the guidelines

• The goal of the guidelines is to improve code quality but they are not an answer to everything; use your best judgment and try to preserve consistency when deciding of code organization.

• The current code does not match the guidelines and won't for a long time - but new code should try to take them into account as much as possible and refactoring will happen progressively.

• These guidelines should be updated as needed.

Code organization

• Backend module code should be organized with services, the only exceptions being the rest.impl classes and utility classes like rest.util.

• Dependencies of a particular service are passed in the constructor so the constructor can be used in unit tests; @Autowired should only be used in rest.impl classes (this is mostly the case already but there are exceptions like InvoiceHelper).

• There is an "API" service for each rest.impl class, in org.folio.services.api (instead of "helper" that are not tied to one API) and we should avoid putting too much logic in these classes. This should help to split helper classes with too many dependencies like PurchaseOrderHelper, and to test all logic without using REST calls. Methods in API services should use different arguments than in rest.impl (RequestContext instead of okapiHeaders, returning a Future instead of using the async handler). API services should not be called by other services; if some functionality in one needs to be called by another service, that functionality should be put into a non-API service.

• Classes in org.folio.rest.impl (implementing RMB generated interfaces) should only do 1 line calls to their matching API services (BaseApi will have a method helping with creating a RequestContext, response generation and basic API logging) or use PgUtil. This will reduce issues with test coverage (it might not be needed to test these classes with unit tests).

• Rather than having a “flow" or "helper" package, we should organize flow as sub-packages of service based on business logic. For instance service.finance.transactions to deal with transactions, or service.pieces to deal with piece receiving flows. It is already organized mostly like that, but we should move more logic from the helper classes into these services.

• We should separate services that do REST calls to other modules (in the same way we separate DAO classes for database calls, but without the need for interfaces) in a service.external package. These classes could benefit from inheriting from a base class to handle methods like getXByIds() using chunks. These classes are likely to be called from unrelated flows.

Unit tests

• New tests should not use MockServer. MockServer will be deprecated.

• Unit tests should only use services and should not do real REST calls.

• Unit tests for some code should generally use a minimal number of services. For instance if code is changed in service3, called by service2, called by service1, a new unit test should only use service3. The exception is when we want to test high-level and low level functionality together; this can be done by instantiating a high-level service and the services it uses, and mocking just the database and/or network calls.

• Unit test packages and classes should match the services they test (for instance the test class for org.folio.service.orders.CompositePoLineValidationService should be org.folio.service.orders.CompositePoLineValidationServiceTest).

• Necessary network and database calls should be mocked, so Futures are returned immediately; there can be exceptions when testing database functionality such as checking a conflict error will be returned by the database when testing optimistic locking, but this should be tested in DAO services unit tests only (using the lowest possible level of testing).

• Test data files should not be too specific to particular tests. For small data it is better to create the structure inline to reduce dependencies between tests and keep test data and code together; for large data a general example can be retrieved from a test data file and modified to suit a particular test.

Karate tests

• New Karate tests must be created in accordance with the best practises and guidelines as outlined by Karate Coding Standards & Best Practices. Existing tests must be refactored to met the new guidelines for the purpose of reducing the growing technical debt.

Other

• To reduce the number of database transactions and improve error handling, DAO classes should always use a Conn or DBConn which is created in a service.