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 <sara...@muellners.com> 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 <jamespdai...@gmail.com> > 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 <m...@vorburger.ch> >> 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. >
