FINERACT-1047 Integration test for Audit trails(Create)

[thesmallstar]

FINERACT-1047

Description

Integration Tests are divided into two parts:

1. Creation
2. Verifying for different routes provided, like: limit,orderby etc.

This PR deals with the first part.

Checklist

Please make sure these boxes are checked before submitting your pull request - thanks!

• Commit message starts with the issue number from https://issues.apache.org/jira/projects/FINERACT/. Ex: FINERACT-646 Pockets API.

• Coding conventions at https://cwiki.apache.org/confluence/display/FINERACT/Coding+Conventions have been followed.

• API documentation at fineract-provider/src/main/resources/static/api-docs/apiLive.htm has been updated with details of any API changes.

• Integration tests have been created/updated for verifying the changes made.

• All Integrations tests are passing with the new commits.

• Submission is not a "code dump". (Large changes can be made "in repository" via a branch. Ask on the list.)

Our guidelines for code reviews is at https://cwiki.apache.org/confluence/display/FINERACT/Code+Review+Guide

[vorburger]

@thesmallstar I have some minor feedback. This is more about improving style of ITs.

[vorburger]

very minor feedback FYI, more for your learning: We usually use the abstract List instead of the concrete ArrayList implementation as type of the variable. Even better, since we are now on Java 11, why not start using var instead? Try just var responseAuditIDs = Utils.performServerGet(requestSpec, responseSpec, AUDIT_URL, "id"); (and don't use final var, that's just ugly).

Suggested change

	final ArrayList<Integer> responseAuditIDs = Utils.performServerGet(requestSpec, responseSpec, AUDIT_URL, "id");
	final List<Integer> responseAuditIDs = Utils.performServerGet(requestSpec, responseSpec, AUDIT_URL, "id");

[thesmallstar]

understood!

[vorburger]

For a new class like AuditHelper, I would go with the pattern of passing this.requestSpec, this.responseSpec to the constructor, and then have a method like verifyAuditCreatedOnServer not require it. Actually, even better, how about only passing the requestSpec and even just hard-code the responseSpec without having to pass it. This would make ITs more readable.

Suggested change

	AuditHelper.verifyAuditCreatedOnServer(this.requestSpec, this.responseSpec, clientId, "CREATE", "CLIENT");
	AuditHelper.verifyAuditCreatedOnServer(this.requestSpec, this.responseSpec, clientId, "CREATE", "CLIENT");

[thesmallstar]

I was actually going to point this on #1019, HideUtilityClassConstructor checks if it is utility class by its name, to do this(using non-private constructor) we would need to either suppress the warning for that checkstyle then or call this something that does not have Helper or util in its name. What do you suggest?

[thesmallstar]

@vorburger I was a little worried about this approach though and thought of changing this to something more robust.

The problem:
We are checking count for the entries like this: Get me the count of audit trails whose resource-id is x it was activated and is of type client. Here, the same event can occur twice and hence two audits will be returned, which does not necessarily mean that the test failed(since we are checking count to be equal to one)

The solution:
Calculate the count before the action(create, update, activate) and check if count incremented by one, after the action.

Why the old approach is still correct?
Two reasons: Every entity is created for the first time, no action is performed twice.

Just that I thought it was fundamentally wrong to check count to be one(not a general case), I hope I am not too confusing. Which approach will you prefer?

[vorburger]

@thesmallstar yes that's a very good point.. you are absolutely right, of course, this test could fail if it runs against a DB that already contains data, for example if it runs after other tests, which makes it potentially flaky and brittle. Change it like you suggested! Maybe instead of only testing that it's +1 more, you could assert on the content of the audit record itself as well?

PS: There's a bigger issue about test isolation underlying this, but we're not going to solve this here.

[vorburger]

improve as discussed

[thesmallstar]

@vorburger I have made the suggested changes. I am not supposed to user var in this case(right?).