Re: Thoughts on updating Flyway Gradle dependency in order to isolate state between integration tests?

[Michael Vorburger]

Dylan,

sorry I'm late to jump on this thread, but I do have some thoughts re. this
I wanted to share:

Could I suggest that you tackle this in two rounds? A first quick and dirty
immediate fix, and then start looking into a more general (and bigger) idea?

For the very short term, I would have to agree with Vishwas that if you can
come up with something  quick and dirty in GlobalConfigurationTest to
update the configuration back to its original value, to fix
https://issues.apache.org/jira/browse/FINERACT-722, that would be... great
progress. Life isn't always perfect, and sometimes in software development
we have to take short cuts... ;-)

As for future follow-up actions/project, once you have contributed a quick
fix for FINERACT-722: So that @FlywayTest annotation is interesting, but I
think there's something else even more suitable here - and much faster (re.
Vishwas that a "full DB re-init" is, relatively, costly). Have you seen, or
are you interested in learning about, how Spring Framework's Integration
Test infrastructure can run tests in transactions which are aborted instead
of commited at the end of integration tests? That's what we really need
here, IMHO.

The minor problem is that we're not quite there yet to be able to benefit
from this, and perhaps other, features of Spring Framework's (great)
Integration Test infrastructure. We may be fairly close though - would you
have any interest in having a look at and helping to finish up
https://issues.apache.org/jira/browse/FINERACT-764 ? That would laid the
foundation for then being able to look into that (above).

BTW: Runtime.getRuntime.exec(“./gradlew migrateTaskName”) wouldn't be my
first choice... :) FYI we're hoping to eventually remove those Gradle
tasks, watch https://issues.apache.org/jira/browse/FINERACT-765. But even
if we weren't, it would be much better to just use the Fineract embedded
Flyway, which runs at boot, programmatically, instead of "forking out" to
the build - does that make sense? Except that, based on above, I don't
think we actually need that. But just in case you ever run into a case
where you really do want to Runtime.getRuntime.exec(), may I shamelessly
plug https://github.com/vorburger/ch.vorburger.exec ? ;-)

Lastly, you can, of course, look into upgrading Flyway, but I would do that
completely separately (and perhaps only after FINERACT-765).

Hope this information is useful. We look very much forward to your
contributions!

Regards,
M.
___
Michael Vorburger
http://www.vorburger.ch


On Sat, Jun 22, 2019 at 9:31 AM Vishwas Babu <
vish...@confluxtechnologies.com> wrote:

> Dylan,
>
> Good job on identifying the root cause :)
>
> While upgrading Flyway is definitely a good idea (purely from the
> perspective of having the latest versions of dependencies), it probably
> isn't required for the problem you are trying to solve.
>
> Restoring the entire database (~350 Migrations) before running a TestSuite
> would significantly increase the time taken for the Integration tests to
> run. This could lead to other problems, ex: Travis times out jobs running
> for more than 50 mins on public repos.
>
> Would it be a better idea to fix the GlobalConfigurationTest instead ? The
> change required would be to ensure that upon successful completion, this
> test case has not modified any configurations (which is causing other test
> cases to fail),
> -> i.e update a configuration
> -> validate it has been updated
> -> then update the configuration back to its original value and validate
> the same
>
> Regards,
> Vishwas
>
>
>
> On Fri, Jun 21, 2019 at 7:59 AM dylanrobson 
> wrote:
>
>> I forgot to mention that I have seen the open PR about upgrading Flyway
>> at https://github.com/apache/fineract/pull/550
>>
>

-- Forwarded message -
From: dylanrobson 
Date: Fri, Jun 21, 2019 at 4:47 PM
Subject: Thoughts on updating Flyway Gradle dependency in order to isolate
state between integration tests?
To: 
Cc: courage angeh , Rahul Goel <
rahul.usi...@gmail.com>


Hello everyone,

Recently I have been researching
https://issues.apache.org/jira/browse/FINERACT-722 about integration
test(s) failing on the first of the month due to an incorrect value of the
loan interest due.

The most interesting thing I found was that when I ran the entire
integration test suite on the first day of the month,
LoanReschedulingWithinCenterTest would always fail.
But when I ran LoanReschedulingWithinCenterTest alone it would always
succeed.

This made me think that somehow state was being “dirtied” between
integration tests.
After lots of debugging I found the root cause was because some values used
to calculate the interest due were read from the global configuration table
(c_configuration).
I found that GloalConfigur

Re: Thoughts on updating Flyway Gradle dependency in order to isolate state between integration tests?

[Vishwas Babu]

Dylan,

Good job on identifying the root cause :)

While upgrading Flyway is definitely a good idea (purely from the
perspective of having the latest versions of dependencies), it probably
isn't required for the problem you are trying to solve.

Restoring the entire database (~350 Migrations) before running a TestSuite
would significantly increase the time taken for the Integration tests to
run. This could lead to other problems, ex: Travis times out jobs running
for more than 50 mins on public repos.

Would it be a better idea to fix the GlobalConfigurationTest instead ? The
change required would be to ensure that upon successful completion, this
test case has not modified any configurations (which is causing other test
cases to fail),
-> i.e update a configuration
-> validate it has been updated
-> then update the configuration back to its original value and validate
the same

Regards,
Vishwas



On Fri, Jun 21, 2019 at 7:59 AM dylanrobson  wrote:

> I forgot to mention that I have seen the open PR about upgrading Flyway at
> https://github.com/apache/fineract/pull/550
> With that in mind should I just try to find a way to accomplish this with
> the current Flyway version?
>

Re: Thoughts on updating Flyway Gradle dependency in order to isolate state between integration tests?

[dylanrobson]

I forgot to mention that I have seen the open PR about upgrading Flyway at 
https://github.com/apache/fineract/pull/550 

With that in mind should I just try to find a way to accomplish this with the 
current Flyway version?

Thoughts on updating Flyway Gradle dependency in order to isolate state between integration tests?

[dylanrobson]

Hello everyone,

Recently I have been researching 
https://issues.apache.org/jira/browse/FINERACT-722 
 about integration test(s) 
failing on the first of the month due to an incorrect value of the loan 
interest due.

The most interesting thing I found was that when I ran the entire integration 
test suite on the first day of the month, LoanReschedulingWithinCenterTest 
would always fail.
But when I ran LoanReschedulingWithinCenterTest alone it would always succeed. 

This made me think that somehow state was being “dirtied” between integration 
tests.
After lots of debugging I found the root cause was because some values used to 
calculate the interest due were read from the global configuration table 
(c_configuration).
I found that GloalConfigurationTest (and a couple other tests) modified this 
table - significantly, the configuration about loan repayments on the first of 
the month was modified.

My first implementation to fix this was simply hardcoding REST API requests in 
the integration test to reset these configurations. 
It worked, but I worried about how well this would work as a long term solution.
- What if these default values change? - A future programmer would have to make 
sure these were all the current default values.
- This only reset the c_configuration table - What if other tests modify the DB 
such that it affects the outcome of other tests? - We would also have to reset 
those values manually and keep them in sync with their defaults manually.

I then found that the DB’s initial instance defaults are the same as at 
fineract-provider/src/main/resources/sql/migrations/sample_data/barebones_db.sql
And I realized that the Grade Flyway migration task initializes these values to 
the same defaults.

I tried to run Fineract's existing Gradle tasks (from within integration test 
@Before functions) using Runtime.getRuntime.exec(“./gradlew migrateTaskName”) 
so that table instance data would be reset.
I found this approach better than hardcoding the values because they would be 
read from the migrations directory and so they’d always be up to date.
However, I found problems with the reliability of this implementation (also 
running gradlew tasks from within Java still felt hacky to me).

Finally, I learned of a newer Flyway feature that could resolve this very 
cleanly and simply. This feature is an annotation: @FlywayTest which can be 
placed on a test class to clean (drop) and migrate the DB to a fresh state. 
The @FlywayTest annotation can also be placed on a function so that this 
clean/migration is done before each test function rather than once before the 
entire class.
See https://github.com/flyway/flyway-test-extensions 


In Fineract, the current Flyway dependency is com.googlecode.flyway:flyway-core 
version 2.1.1 (last updated March 2013).
See https://mvnrepository.com/artifact/com.googlecode.flyway/flyway-core 

com.googlecode.flyway had it’s last update December 2013, and says it’s now 
moved to org.flywaydb:flyway-core version 5.x and 6.x (6.x is beta but has 
updates in 2019)
See https://mvnrepository.com/artifact/org.flywaydb/flyway-core 


This new Flyway feature (@FlywayTest) is only available from the org.flywaydb 
repo (not the current Fineract dependency com.googlecode.flyway).

So my final question: 

Would it be worthwhile to update / refactor Flyway so I could use this feature, 
or do you think it would be unnecessary work?
If it would be unnecessary /  too time consuming to update this Flyway 
dependency, I could try to find a way to achieve this using Fineract's current 
version of the Flyway API.

Please let me know your thoughts / concerns.

Thanks in advance for your time and advice
— Dylan Robson