Re: [PATCH v3 06/19] test: Avoid failing skipped tests
On Wed, Jun 26, 2024 at 07:56:24AM -0600, Tom Rini wrote:
> On Wed, Jun 26, 2024 at 09:00:42AM +0100, Simon Glass wrote:
> > Hi Tom,
> >
> > On Tue, 25 Jun 2024 at 15:14, Tom Rini wrote:
> > >
> > > On Tue, Jun 25, 2024 at 01:38:00PM +0100, Simon Glass wrote:
> > > > Hi Tom,
> > > >
> > > > On Mon, 24 Jun 2024 at 19:06, Tom Rini wrote:
> > > > >
> > > > > On Sun, Jun 23, 2024 at 02:32:00PM -0600, Simon Glass wrote:
> > > > >
> > > > > > When a test returns -EAGAIN this should not be considered a failure.
> > > > > > Fix what seems to be a problem case, where the pytests see a failure
> > > > > > when a test has merely been skipped.
> > > > > >
> > > > > > Signed-off-by: Simon Glass
> > > > > > ---
> > > > > >
> > > > > > (no changes since v1)
> > > > > >
> > > > > > test/test-main.c | 16 +++-
> > > > > > 1 file changed, 11 insertions(+), 5 deletions(-)
> > > > > >
> > > > > > diff --git a/test/test-main.c b/test/test-main.c
> > > > > > index 3fa6f6e32ec..cda1a186390 100644
> > > > > > --- a/test/test-main.c
> > > > > > +++ b/test/test-main.c
> > > > > > @@ -448,7 +448,7 @@ static int ut_run_test(struct unit_test_state
> > > > > > *uts, struct unit_test *test,
> > > > > > static int ut_run_test_live_flat(struct unit_test_state *uts,
> > > > > >struct unit_test *test)
> > > > > > {
> > > > > > - int runs;
> > > > > > + int runs, ret;
> > > > > >
> > > > > > if ((test->flags & UT_TESTF_OTHER_FDT) &&
> > > > > > !IS_ENABLED(CONFIG_SANDBOX))
> > > > > > return skip_test(uts);
> > > > > > @@ -458,8 +458,11 @@ static int ut_run_test_live_flat(struct
> > > > > > unit_test_state *uts,
> > > > > > if (CONFIG_IS_ENABLED(OF_LIVE)) {
> > > > > > if (!(test->flags & UT_TESTF_FLAT_TREE)) {
> > > > > > uts->of_live = true;
> > > > > > - ut_assertok(ut_run_test(uts, test,
> > > > > > test->name));
> > > > > > - runs++;
> > > > > > + ret = ut_run_test(uts, test, test->name);
> > > > > > + if (ret != -EAGAIN) {
> > > > > > + ut_assertok(ret);
> > > > > > + runs++;
> > > > > > + }
> > > > > > }
> > > > > > }
> > > > > >
> > > > > > @@ -483,8 +486,11 @@ static int ut_run_test_live_flat(struct
> > > > > > unit_test_state *uts,
> > > > > > (!runs || ut_test_run_on_flattree(test)) &&
> > > > > > !(gd->flags & GD_FLG_FDT_CHANGED)) {
> > > > > > uts->of_live = false;
> > > > > > - ut_assertok(ut_run_test(uts, test, test->name));
> > > > > > - runs++;
> > > > > > + ret = ut_run_test(uts, test, test->name);
> > > > > > + if (ret != -EAGAIN) {
> > > > > > + ut_assertok(ret);
> > > > > > + runs++;
> > > > > > + }
> > > > > > }
> > > > > >
> > > > > > return 0;
> > > > >
> > > > > How did you trigger this case exactly?
> > > >
> > > > I noticed this in CI, where some skipped tests were shown as failed in
> > > > the log, even though they were not counted as failures in the final
> > > > results.
> > >
> > > That's really really strange, do you have an example log or something
> > > around still?
> >
> > This happens on snow, which is (maybe) the only real board that
> > defines CONFIG_UNIT_TEST
>
> I think it is too, but that's also perhaps a reminder that I should be
> enabling it as part of my build before testing scripts. I'll go do that
> now and see if this problem shows up a tiny bit more widely.
OK, not right now then, there's missing dependencies within the test.
I'll selectively enable once v2 is in tho.
--
Tom
signature.asc
Description: PGP signature
Re: [PATCH v3 06/19] test: Avoid failing skipped tests
On Wed, Jun 26, 2024 at 09:00:42AM +0100, Simon Glass wrote:
> Hi Tom,
>
> On Tue, 25 Jun 2024 at 15:14, Tom Rini wrote:
> >
> > On Tue, Jun 25, 2024 at 01:38:00PM +0100, Simon Glass wrote:
> > > Hi Tom,
> > >
> > > On Mon, 24 Jun 2024 at 19:06, Tom Rini wrote:
> > > >
> > > > On Sun, Jun 23, 2024 at 02:32:00PM -0600, Simon Glass wrote:
> > > >
> > > > > When a test returns -EAGAIN this should not be considered a failure.
> > > > > Fix what seems to be a problem case, where the pytests see a failure
> > > > > when a test has merely been skipped.
> > > > >
> > > > > Signed-off-by: Simon Glass
> > > > > ---
> > > > >
> > > > > (no changes since v1)
> > > > >
> > > > > test/test-main.c | 16 +++-
> > > > > 1 file changed, 11 insertions(+), 5 deletions(-)
> > > > >
> > > > > diff --git a/test/test-main.c b/test/test-main.c
> > > > > index 3fa6f6e32ec..cda1a186390 100644
> > > > > --- a/test/test-main.c
> > > > > +++ b/test/test-main.c
> > > > > @@ -448,7 +448,7 @@ static int ut_run_test(struct unit_test_state
> > > > > *uts, struct unit_test *test,
> > > > > static int ut_run_test_live_flat(struct unit_test_state *uts,
> > > > >struct unit_test *test)
> > > > > {
> > > > > - int runs;
> > > > > + int runs, ret;
> > > > >
> > > > > if ((test->flags & UT_TESTF_OTHER_FDT) &&
> > > > > !IS_ENABLED(CONFIG_SANDBOX))
> > > > > return skip_test(uts);
> > > > > @@ -458,8 +458,11 @@ static int ut_run_test_live_flat(struct
> > > > > unit_test_state *uts,
> > > > > if (CONFIG_IS_ENABLED(OF_LIVE)) {
> > > > > if (!(test->flags & UT_TESTF_FLAT_TREE)) {
> > > > > uts->of_live = true;
> > > > > - ut_assertok(ut_run_test(uts, test, test->name));
> > > > > - runs++;
> > > > > + ret = ut_run_test(uts, test, test->name);
> > > > > + if (ret != -EAGAIN) {
> > > > > + ut_assertok(ret);
> > > > > + runs++;
> > > > > + }
> > > > > }
> > > > > }
> > > > >
> > > > > @@ -483,8 +486,11 @@ static int ut_run_test_live_flat(struct
> > > > > unit_test_state *uts,
> > > > > (!runs || ut_test_run_on_flattree(test)) &&
> > > > > !(gd->flags & GD_FLG_FDT_CHANGED)) {
> > > > > uts->of_live = false;
> > > > > - ut_assertok(ut_run_test(uts, test, test->name));
> > > > > - runs++;
> > > > > + ret = ut_run_test(uts, test, test->name);
> > > > > + if (ret != -EAGAIN) {
> > > > > + ut_assertok(ret);
> > > > > + runs++;
> > > > > + }
> > > > > }
> > > > >
> > > > > return 0;
> > > >
> > > > How did you trigger this case exactly?
> > >
> > > I noticed this in CI, where some skipped tests were shown as failed in
> > > the log, even though they were not counted as failures in the final
> > > results.
> >
> > That's really really strange, do you have an example log or something
> > around still?
>
> This happens on snow, which is (maybe) the only real board that
> defines CONFIG_UNIT_TEST
I think it is too, but that's also perhaps a reminder that I should be
enabling it as part of my build before testing scripts. I'll go do that
now and see if this problem shows up a tiny bit more widely.
> test/py/tests/test_ut.py sssnow # ut bdinfo bdinfo_test_eth
> Test: bdinfo_test_eth: bdinfo.c
> Skipping: Console recording disabled
> test/test-main.c:486, ut_run_test_live_flat(): 0 == ut_run_test(uts,
> test, test->name): Expected 0x0 (0), got 0xfff5 (-11)
> Test bdinfo_test_eth failed 1 times
> Skipped: 1, Failures: 1
> snow # F+u-boot-test-reset snow snow
>
> For this particular mechanism (-EAGAIN returned by test_pre_run()) , I
> think a better fix would be to squash the error in ut_run_test(), as
> is done when -EAGAIN is returned in the body of the test. I'll update
> that. I cannot see any other way this could happen, but we can always
> deal with it later if it does.
Thanks for explaining, please also include the example in the commit
message in v2.
--
Tom
signature.asc
Description: PGP signature
Re: [PATCH v3 06/19] test: Avoid failing skipped tests
Hi Tom,
On Tue, 25 Jun 2024 at 15:14, Tom Rini wrote:
>
> On Tue, Jun 25, 2024 at 01:38:00PM +0100, Simon Glass wrote:
> > Hi Tom,
> >
> > On Mon, 24 Jun 2024 at 19:06, Tom Rini wrote:
> > >
> > > On Sun, Jun 23, 2024 at 02:32:00PM -0600, Simon Glass wrote:
> > >
> > > > When a test returns -EAGAIN this should not be considered a failure.
> > > > Fix what seems to be a problem case, where the pytests see a failure
> > > > when a test has merely been skipped.
> > > >
> > > > Signed-off-by: Simon Glass
> > > > ---
> > > >
> > > > (no changes since v1)
> > > >
> > > > test/test-main.c | 16 +++-
> > > > 1 file changed, 11 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/test/test-main.c b/test/test-main.c
> > > > index 3fa6f6e32ec..cda1a186390 100644
> > > > --- a/test/test-main.c
> > > > +++ b/test/test-main.c
> > > > @@ -448,7 +448,7 @@ static int ut_run_test(struct unit_test_state *uts,
> > > > struct unit_test *test,
> > > > static int ut_run_test_live_flat(struct unit_test_state *uts,
> > > >struct unit_test *test)
> > > > {
> > > > - int runs;
> > > > + int runs, ret;
> > > >
> > > > if ((test->flags & UT_TESTF_OTHER_FDT) &&
> > > > !IS_ENABLED(CONFIG_SANDBOX))
> > > > return skip_test(uts);
> > > > @@ -458,8 +458,11 @@ static int ut_run_test_live_flat(struct
> > > > unit_test_state *uts,
> > > > if (CONFIG_IS_ENABLED(OF_LIVE)) {
> > > > if (!(test->flags & UT_TESTF_FLAT_TREE)) {
> > > > uts->of_live = true;
> > > > - ut_assertok(ut_run_test(uts, test, test->name));
> > > > - runs++;
> > > > + ret = ut_run_test(uts, test, test->name);
> > > > + if (ret != -EAGAIN) {
> > > > + ut_assertok(ret);
> > > > + runs++;
> > > > + }
> > > > }
> > > > }
> > > >
> > > > @@ -483,8 +486,11 @@ static int ut_run_test_live_flat(struct
> > > > unit_test_state *uts,
> > > > (!runs || ut_test_run_on_flattree(test)) &&
> > > > !(gd->flags & GD_FLG_FDT_CHANGED)) {
> > > > uts->of_live = false;
> > > > - ut_assertok(ut_run_test(uts, test, test->name));
> > > > - runs++;
> > > > + ret = ut_run_test(uts, test, test->name);
> > > > + if (ret != -EAGAIN) {
> > > > + ut_assertok(ret);
> > > > + runs++;
> > > > + }
> > > > }
> > > >
> > > > return 0;
> > >
> > > How did you trigger this case exactly?
> >
> > I noticed this in CI, where some skipped tests were shown as failed in
> > the log, even though they were not counted as failures in the final
> > results.
>
> That's really really strange, do you have an example log or something
> around still?
This happens on snow, which is (maybe) the only real board that
defines CONFIG_UNIT_TEST
test/py/tests/test_ut.py sssnow # ut bdinfo bdinfo_test_eth
Test: bdinfo_test_eth: bdinfo.c
Skipping: Console recording disabled
test/test-main.c:486, ut_run_test_live_flat(): 0 == ut_run_test(uts,
test, test->name): Expected 0x0 (0), got 0xfff5 (-11)
Test bdinfo_test_eth failed 1 times
Skipped: 1, Failures: 1
snow # F+u-boot-test-reset snow snow
For this particular mechanism (-EAGAIN returned by test_pre_run()) , I
think a better fix would be to squash the error in ut_run_test(), as
is done when -EAGAIN is returned in the body of the test. I'll update
that. I cannot see any other way this could happen, but we can always
deal with it later if it does.
Regards,
Simon
Re: [PATCH v3 06/19] test: Avoid failing skipped tests
On Tue, Jun 25, 2024 at 01:38:00PM +0100, Simon Glass wrote:
> Hi Tom,
>
> On Mon, 24 Jun 2024 at 19:06, Tom Rini wrote:
> >
> > On Sun, Jun 23, 2024 at 02:32:00PM -0600, Simon Glass wrote:
> >
> > > When a test returns -EAGAIN this should not be considered a failure.
> > > Fix what seems to be a problem case, where the pytests see a failure
> > > when a test has merely been skipped.
> > >
> > > Signed-off-by: Simon Glass
> > > ---
> > >
> > > (no changes since v1)
> > >
> > > test/test-main.c | 16 +++-
> > > 1 file changed, 11 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/test/test-main.c b/test/test-main.c
> > > index 3fa6f6e32ec..cda1a186390 100644
> > > --- a/test/test-main.c
> > > +++ b/test/test-main.c
> > > @@ -448,7 +448,7 @@ static int ut_run_test(struct unit_test_state *uts,
> > > struct unit_test *test,
> > > static int ut_run_test_live_flat(struct unit_test_state *uts,
> > >struct unit_test *test)
> > > {
> > > - int runs;
> > > + int runs, ret;
> > >
> > > if ((test->flags & UT_TESTF_OTHER_FDT) &&
> > > !IS_ENABLED(CONFIG_SANDBOX))
> > > return skip_test(uts);
> > > @@ -458,8 +458,11 @@ static int ut_run_test_live_flat(struct
> > > unit_test_state *uts,
> > > if (CONFIG_IS_ENABLED(OF_LIVE)) {
> > > if (!(test->flags & UT_TESTF_FLAT_TREE)) {
> > > uts->of_live = true;
> > > - ut_assertok(ut_run_test(uts, test, test->name));
> > > - runs++;
> > > + ret = ut_run_test(uts, test, test->name);
> > > + if (ret != -EAGAIN) {
> > > + ut_assertok(ret);
> > > + runs++;
> > > + }
> > > }
> > > }
> > >
> > > @@ -483,8 +486,11 @@ static int ut_run_test_live_flat(struct
> > > unit_test_state *uts,
> > > (!runs || ut_test_run_on_flattree(test)) &&
> > > !(gd->flags & GD_FLG_FDT_CHANGED)) {
> > > uts->of_live = false;
> > > - ut_assertok(ut_run_test(uts, test, test->name));
> > > - runs++;
> > > + ret = ut_run_test(uts, test, test->name);
> > > + if (ret != -EAGAIN) {
> > > + ut_assertok(ret);
> > > + runs++;
> > > + }
> > > }
> > >
> > > return 0;
> >
> > How did you trigger this case exactly?
>
> I noticed this in CI, where some skipped tests were shown as failed in
> the log, even though they were not counted as failures in the final
> results.
That's really really strange, do you have an example log or something
around still?
--
Tom
signature.asc
Description: PGP signature
Re: [PATCH v3 06/19] test: Avoid failing skipped tests
Hi Tom,
On Mon, 24 Jun 2024 at 19:06, Tom Rini wrote:
>
> On Sun, Jun 23, 2024 at 02:32:00PM -0600, Simon Glass wrote:
>
> > When a test returns -EAGAIN this should not be considered a failure.
> > Fix what seems to be a problem case, where the pytests see a failure
> > when a test has merely been skipped.
> >
> > Signed-off-by: Simon Glass
> > ---
> >
> > (no changes since v1)
> >
> > test/test-main.c | 16 +++-
> > 1 file changed, 11 insertions(+), 5 deletions(-)
> >
> > diff --git a/test/test-main.c b/test/test-main.c
> > index 3fa6f6e32ec..cda1a186390 100644
> > --- a/test/test-main.c
> > +++ b/test/test-main.c
> > @@ -448,7 +448,7 @@ static int ut_run_test(struct unit_test_state *uts,
> > struct unit_test *test,
> > static int ut_run_test_live_flat(struct unit_test_state *uts,
> >struct unit_test *test)
> > {
> > - int runs;
> > + int runs, ret;
> >
> > if ((test->flags & UT_TESTF_OTHER_FDT) && !IS_ENABLED(CONFIG_SANDBOX))
> > return skip_test(uts);
> > @@ -458,8 +458,11 @@ static int ut_run_test_live_flat(struct
> > unit_test_state *uts,
> > if (CONFIG_IS_ENABLED(OF_LIVE)) {
> > if (!(test->flags & UT_TESTF_FLAT_TREE)) {
> > uts->of_live = true;
> > - ut_assertok(ut_run_test(uts, test, test->name));
> > - runs++;
> > + ret = ut_run_test(uts, test, test->name);
> > + if (ret != -EAGAIN) {
> > + ut_assertok(ret);
> > + runs++;
> > + }
> > }
> > }
> >
> > @@ -483,8 +486,11 @@ static int ut_run_test_live_flat(struct
> > unit_test_state *uts,
> > (!runs || ut_test_run_on_flattree(test)) &&
> > !(gd->flags & GD_FLG_FDT_CHANGED)) {
> > uts->of_live = false;
> > - ut_assertok(ut_run_test(uts, test, test->name));
> > - runs++;
> > + ret = ut_run_test(uts, test, test->name);
> > + if (ret != -EAGAIN) {
> > + ut_assertok(ret);
> > + runs++;
> > + }
> > }
> >
> > return 0;
>
> How did you trigger this case exactly?
I noticed this in CI, where some skipped tests were shown as failed in
the log, even though they were not counted as failures in the final
results.
>
> --
> Tom
Re: [PATCH v3 06/19] test: Avoid failing skipped tests
On Sun, Jun 23, 2024 at 02:32:00PM -0600, Simon Glass wrote:
> When a test returns -EAGAIN this should not be considered a failure.
> Fix what seems to be a problem case, where the pytests see a failure
> when a test has merely been skipped.
>
> Signed-off-by: Simon Glass
> ---
>
> (no changes since v1)
>
> test/test-main.c | 16 +++-
> 1 file changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/test/test-main.c b/test/test-main.c
> index 3fa6f6e32ec..cda1a186390 100644
> --- a/test/test-main.c
> +++ b/test/test-main.c
> @@ -448,7 +448,7 @@ static int ut_run_test(struct unit_test_state *uts,
> struct unit_test *test,
> static int ut_run_test_live_flat(struct unit_test_state *uts,
>struct unit_test *test)
> {
> - int runs;
> + int runs, ret;
>
> if ((test->flags & UT_TESTF_OTHER_FDT) && !IS_ENABLED(CONFIG_SANDBOX))
> return skip_test(uts);
> @@ -458,8 +458,11 @@ static int ut_run_test_live_flat(struct unit_test_state
> *uts,
> if (CONFIG_IS_ENABLED(OF_LIVE)) {
> if (!(test->flags & UT_TESTF_FLAT_TREE)) {
> uts->of_live = true;
> - ut_assertok(ut_run_test(uts, test, test->name));
> - runs++;
> + ret = ut_run_test(uts, test, test->name);
> + if (ret != -EAGAIN) {
> + ut_assertok(ret);
> + runs++;
> + }
> }
> }
>
> @@ -483,8 +486,11 @@ static int ut_run_test_live_flat(struct unit_test_state
> *uts,
> (!runs || ut_test_run_on_flattree(test)) &&
> !(gd->flags & GD_FLG_FDT_CHANGED)) {
> uts->of_live = false;
> - ut_assertok(ut_run_test(uts, test, test->name));
> - runs++;
> + ret = ut_run_test(uts, test, test->name);
> + if (ret != -EAGAIN) {
> + ut_assertok(ret);
> + runs++;
> + }
> }
>
> return 0;
How did you trigger this case exactly?
--
Tom
signature.asc
Description: PGP signature
