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 < [email protected]> 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 <[email protected]> > 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 <[email protected]> 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: <[email protected]> Cc: courage angeh <[email protected]>, Rahul Goel < [email protected]> 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 > With that in mind should I just try to find a way to accomplish this with >> the current Flyway version? >> >
