----- Original Message ----- > From: "Jeff Layton" <jlayton@xxxxxxxxxx> > To: "Frank Filz" <ffilzlnx@xxxxxxxxxxxxxx>, "J. Bruce Fields" <bfields@xxxxxxxxxxxx>, "Dai Ngo" <dai.ngo@xxxxxxxxxx> > Cc: "linux-nfs" <linux-nfs@xxxxxxxxxxxxxxx> > Sent: Thursday, 23 February, 2023 18:08:19 > Subject: Re: [pynfs RFC PATCH] nfs4.0/testserver.py: don't return an error when tests fail > On Thu, 2023-02-23 at 08:22 -0800, Frank Filz wrote: >> > From: Jeff Layton [mailto:jlayton@xxxxxxxxxx] >> >> > This script was originally changed in eb3ba0b60055 ("Have >> > testserver.py >> have >> > non-zero exit code if any tests fail"), but the same change wasn't >> > made to >> the >> > 4.1 testserver.py script. >> > >> > There also wasn't much explanation for it, and it makes it difficult >> > to >> tell >> > whether the test harness itself failed, or whether there was a >> > failure in >> a >> > requested test. >> > >> > Stop the 4.0 testserver.py from exiting with an error code when a >> > test >> fails, so >> > that a successful return means only that the test harness itself >> > worked, >> not that >> > every requested test passed. >> > >> > Cc: Frank Filz <ffilzlnx@xxxxxxxxxxxxxx> >> > Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx> >> > --- >> > nfs4.0/testserver.py | 2 -- >> > 1 file changed, 2 deletions(-) >> > >> > I'm not sure about this one. I've worked around this in kdevops for >> > now, >> but it >> > would really be preferable if it worked this way, imo. If this isn't >> acceptable, >> > maybe we can add a new option that enables this behavior? >> > >> > Frank, what was the original rationale for eb3ba0b60055 ? >> >> We needed a way for CI to easily detect failure of pynfs. I'm not sure >> how >> helpful it is since Ganesha does fail some tests... >> >> It might be helpful to have some helpers for CI to use, or an option >> that >> causes pynfs to report in a way that's much easier for CI to determine >> if >> pynfs succeeded or not. >> > > That's exactly the issue I had when working with this for kdevops. It > runs testserver.py via ansible, and when it gets a non-zero exit code, > the run aborts without doing anything further. I have it ignoring the > return code from testserver.py for now, but that's not ideal since I > can't catch actual problems with the test harness. > > I have testserver.py output the result to JSON, and then analyze that to > see if anything failed. That also gives us what you were asking for in > your other email -- the ability to filter out known failures. Here's > what I have so far, but I'd like to expand it to highlight other > behavior changes: > > https://github.com/linux-kdevops/kdevops/blob/master/scripts/workflows/pynfs/check_pynfs_results.py > > It may make sense to move that script into pynfs itself. > > If there is CI that depends on this behavior, then I'd be interested to > hear how they are dealing with failed tests. Do they just not run the > tests that always fail? Same here... The test generates for us xunit report, thus error code is in the reporting and we always have to run it as: ``` ./testserver.py -v --noinit --xml="${WORKSPACE}/xunit-report-v41.xml" ${SUT}:${TEST_PATH} all $NFS41_INCLUDES $NFS41_EXCLUDES_OPTION || true ``` > >> Hmm, one thing that would help is to be able to flag a set of tests >> that >> should not constitute a CI failure (known errors) but we want to keep >> running them because of what they exercise, or to more readily detect >> that >> they have been fixed. yeah, an option might do the job. Tigran. >> > > The right way to do that is the same way that xfstests works. You test > for the conditions being favorable for a test and then just skip it if > they aren't. > >> > diff --git a/nfs4.0/testserver.py b/nfs4.0/testserver.py index >> > f2c41568e5c7..4f4286daa657 100755 >> > --- a/nfs4.0/testserver.py >> > +++ b/nfs4.0/testserver.py >> > @@ -387,8 +387,6 @@ def main(): >> > >> > if nfail < 0: >> > sys.exit(3) >> > - if nfail > 0: >> > - sys.exit(2) >> > >> > if __name__ == "__main__": >> > main() >> > -- >> > 2.39.2 >> > > -- > Jeff Layton <jlayton@xxxxxxxxxx>
Attachment:
smime.p7s
Description: S/MIME Cryptographic Signature
