So I've had some "fun" on this front, and propose https://github.com/apache/fineract/pull/817, which is the first step to completely change how our integration tests tests await completion of background jobs... code review feedback welcome!
On Sun, May 3, 2020 at 1:57 PM Awasum Yannick <[email protected]> wrote: > Full disclosure, Petri first noticed this here > <https://issues.apache.org/jira/browse/FINERACT-855?focusedCommentId=17098277&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-17098277> > > Also see: https://issues.apache.org/jira/browse/FINERACT-922 > > On Sun, May 3, 2020 at 12:18 PM Awasum Yannick <[email protected]> wrote: > >> Looks like the flaky Integration tests failures is as a result of >> schedule jobs taking too much time to complete so much that by the time an >> assertion is done, the job history has zero completed jobs and we get in >> some cases array out of bound exceptions due to 0 - 1 index. And in some >> cases assertion errors. >> >> I will soon be sending a pr based on this and comments made else where. >> >> PS: sorry, am on mobile >> >> On Sat, May 2, 2020, 18:00 Michael Vorburger <[email protected]> wrote: >> >>> Saransh, >>> >>> yes, totally agree, of course fixing any failing test is the best, and >>> any contributions for that would be very helpful. >>> >>> The point we are making with >>> https://github.com/apache/fineract#pull-requests is obviously not ;) >>> that we don't welcome help with fixing flaky tests, but simply that we >>> cannot block people's PRs on anything else due to clearly unstable ITs. A >>> project cannot stop parallel work and just grind to a complete halt because >>> of some failing tests - as important as fixing failing tests ASAP is, yes, >>> of course. >>> >>> I do meanwhile firmly believe that ignoring tests, IFF they are proven >>> to be unrelated to a change proposed in a PR (as the README states), and >>> then having separate investigations by interested parties to re-un-ignore >>> any fixed tests, is the right strategy. Hope you agree? >>> >>> Best, >>> M. >>> _______________________ >>> Michael Vorburger >>> http://www.vorburger.ch >>> >>> >>> On Sat, May 2, 2020 at 6:17 PM Saransh Sharma <[email protected]> >>> wrote: >>> >>>> >>>> I investigated these tests, they are failing because of the >>>> interdependent nature of the application, Ignoring them eventually is not a >>>> better idea. Since the nature of the "flaky" test is unknown , are we sure >>>> that we might not encounter the same problem? in the next PR or in the >>>> future PR's >>>> >>>> Points : >>>> >>>> 1. I made some changes not related to anything to other packages >>>> >>>> Failed 3 of the tests . :) Pretty crazy , I think the status from the >>>> API is also returning 403 (is this normal don't think so.) >>>> >>>> https://api.travis-ci.org/v3/job/682216325/log.txt (Is this normal >>>> behaviour , plus local its quite different) >>>> >>>> Thus leading to this , "The following objects may have been >>>> concurrently modified in another transaction" It is clear that at the same >>>> time the transactions are happening. >>>> I would suggest that we group tests based on the packages and then >>>> perform functional tests based on the order of the dependency. >>>> >>>> Let me know, if at all we should try to resolve those test issues >>>> rather than putting Ignore. >>>> >>>> On Tue, Apr 28, 2020 at 10:00 AM James Dailey <[email protected]> >>>> wrote: >>>> >>>>> +1 >>>>> >>>>> "Please do NOT just @Ignore any existing tests mixed in as part of >>>>> your larger PR." >>>>> >>>>> This seems like a key point to bring up to a higher level in the code >>>>> review. i.e. should there be a search for @Ignore when doing a code >>>>> review? >>>>> >>>>> >>>>> >>>>> >>>>> >>>>> >>>>> >>>>> >>>>> On Sun, Apr 26, 2020 at 2:30 AM Michael Vorburger <[email protected]> >>>>> wrote: >>>>> >>>>>> Hello everyone, >>>>>> >>>>>> I propose https://github.com/apache/fineract/pull/782 as our New >>>>>> Policy about how to deal with unstable tests (text proposed to be added >>>>>> to >>>>>> the README in that PR is copy/pasted below for your reading convenience). >>>>>> >>>>>> Tx, >>>>>> M. >>>>>> >>>>>> If your PR is failing to pass our CI build due to a test failure >>>>>> which you suspect is 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, like this: >>>>>> 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>. 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 @Ignore 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 @Ignore >>>>>> any existing tests mixed in as part of your larger PR. 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. Then (only) >>>>>> Close and Reopen your PR, which will cause a new build, to see if it >>>>>> passes. >>>>>> >>>>>> _______________________ >>>>>> Michael Vorburger >>>>>> http://www.vorburger.ch >>>>>> >>>>> >>>> >>>> -- >>>> >>>> Saransh Sharma >>>> *Research Partner* >>>> *Muellner Internet Pvt Ltd * >>>> >>>> >>>> ---------------------------------------------------------------------------------------------- >>>> >>>> The idea of separation of me and you is amazing. >>>> >>>> ---------------------------------------------------------------------------------------------- >>>> *Company Website <https://www.muellners.com/> **Company Linkedin >>>> <https://www.linkedin.com/company/muellners> * >>>> Company Facebook <https://www.facebook.com/muellners> >>>> >>>> This mail is governed by Muellner® Internet Private Limited's IT >>>> policy. >>>> The information contained in this e-mail and any accompanying documents >>>> may contain information that is confidential or otherwise protected from >>>> disclosure. If you are not the intended recipient of this message, or if >>>> this message has been addressed to you in error, please immediately alert >>>> the sender by reply e-mail and then delete this message, including any >>>> attachments. Any dissemination, distribution or other use of the contents >>>> of this message by anyone other than the intended recipient is strictly >>>> prohibited. All messages sent to and from this e-mail address may be >>>> monitored as permitted by applicable law and regulations to ensure >>>> compliance with our internal policies and to protect our business. E-mails >>>> are not secure and cannot be guaranteed to be error free as they can be >>>> intercepted, amended, lost or destroyed, or contain viruses. You are deemed >>>> to have accepted these risks if you communicate with us by e-mail. >>>> >>>
