| Message ID | 20240229124246.309304-5-cem@kernel.org |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | Add riscv tests to cover the base extension specs | expand |
On Thu, Feb 29, 2024 at 01:42:10PM +0100, cem@kernel.org wrote: > From: Carlos Maiolino <cem@kernel.org> > > If the test fails because the expected value doesn't match, it's useful to know > what value was actually printed. > > Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com> > --- > riscv/sbi.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/riscv/sbi.c b/riscv/sbi.c > index fa28d7c8..8ad8f375 100644 > --- a/riscv/sbi.c > +++ b/riscv/sbi.c > @@ -28,7 +28,12 @@ static bool env_is_defined(const char *env) > static void gen_report(struct sbiret *ret, long expected) > { > report(!ret->error, "no sbi.error"); > - report(ret->value == expected, "expected sbi.value"); > + > + if (ret->value == expected) > + report(true, "expected sbi.value"); > + else > + report(false, "expected sbi.value: %ld - Got: %ld", > + expected, ret->value); We want to keep the output line consistent for both pass and fail in order to simplify parsing. But, since the expected value is variable, depending on the SBI implementation under test, it's probably best not to put the numbers in the pass/fail line, but as info. So how about static void gen_report(struct sbiret *ret, long expected_error, long expected_value) { report_info("expected (error: %ld, value: %ld), received: (error: %ld, value: %ld)", expected_error, expected_value, ret->error, ret->value); report(ret->error == expected_error, "expected sbi.error"); report(ret->value == expected_value, "expected sbi.value"); } I've changed this function to take the error as input because negative tests (which we should also have) will be looking for specific errors. Thanks, drew > } > > static void check_base(void) > -- > 2.43.2 > > > -- > kvm-riscv mailing list > kvm-riscv@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/kvm-riscv
This patch's summary ($SUBJECT) is too long. Thanks, drew On Thu, Feb 29, 2024 at 01:42:10PM +0100, cem@kernel.org wrote: > From: Carlos Maiolino <cem@kernel.org> > > If the test fails because the expected value doesn't match, it's useful to know > what value was actually printed. > > Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com> > --- > riscv/sbi.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/riscv/sbi.c b/riscv/sbi.c > index fa28d7c8..8ad8f375 100644 > --- a/riscv/sbi.c > +++ b/riscv/sbi.c > @@ -28,7 +28,12 @@ static bool env_is_defined(const char *env) > static void gen_report(struct sbiret *ret, long expected) > { > report(!ret->error, "no sbi.error"); > - report(ret->value == expected, "expected sbi.value"); > + > + if (ret->value == expected) > + report(true, "expected sbi.value"); > + else > + report(false, "expected sbi.value: %ld - Got: %ld", > + expected, ret->value); > } > > static void check_base(void) > -- > 2.43.2 > > > -- > kvm-riscv mailing list > kvm-riscv@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/kvm-riscv
> > We want to keep the output line consistent for both pass and fail > in order to simplify parsing. But, since the expected value is > variable, depending on the SBI implementation under test, it's > probably best not to put the numbers in the pass/fail line, but as > info. So how about > > static void gen_report(struct sbiret *ret, > long expected_error, long expected_value) > { > report_info("expected (error: %ld, value: %ld), received: (error: %ld, value: %ld)", > expected_error, expected_value, ret->error, ret->value); > report(ret->error == expected_error, "expected sbi.error"); > report(ret->value == expected_value, "expected sbi.value"); > } Something just came up to my mind while implementing this. I agree with the use of report_info() here, but I still think this could be under a conditional to only print it when we have something different from the expectations. I particularly don't see much value in printing the report_info if nothing unexpected, but maybe I'm wrong? Carlos > > I've changed this function to take the error as input because negative > tests (which we should also have) will be looking for specific errors. > > Thanks, > drew > > > > } > > > > static void check_base(void) > > -- > > 2.43.2 > > > > > > -- > > kvm-riscv mailing list > > kvm-riscv@lists.infradead.org > > http://lists.infradead.org/mailman/listinfo/kvm-riscv > > -- > kvm-riscv mailing list > kvm-riscv@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/kvm-riscv
On Tue, Mar 05, 2024 at 09:05:31AM +0100, Carlos Maiolino wrote: > > > > We want to keep the output line consistent for both pass and fail > > in order to simplify parsing. But, since the expected value is > > variable, depending on the SBI implementation under test, it's > > probably best not to put the numbers in the pass/fail line, but as > > info. So how about > > > > static void gen_report(struct sbiret *ret, > > long expected_error, long expected_value) > > { > > report_info("expected (error: %ld, value: %ld), received: (error: %ld, value: %ld)", > > expected_error, expected_value, ret->error, ret->value); > > report(ret->error == expected_error, "expected sbi.error"); > > report(ret->value == expected_value, "expected sbi.value"); > > } > > Something just came up to my mind while implementing this. > > I agree with the use of report_info() here, but I still think this could be > under a conditional to only print it when we have something different from the > expectations. I particularly don't see much value in printing the report_info if > nothing unexpected, but maybe I'm wrong? I don't mind either way. report_info() is for human readers, so parsers should ignore them and not freak out when they appear or disappear. Not cluttering the output by removing useless info sounds good to me, but also not cluttering the code, by not adding unnecessary logic, sounds good to me. So, it's up to you :-) Thanks, drew
diff --git a/riscv/sbi.c b/riscv/sbi.c index fa28d7c8..8ad8f375 100644 --- a/riscv/sbi.c +++ b/riscv/sbi.c @@ -28,7 +28,12 @@ static bool env_is_defined(const char *env) static void gen_report(struct sbiret *ret, long expected) { report(!ret->error, "no sbi.error"); - report(ret->value == expected, "expected sbi.value"); + + if (ret->value == expected) + report(true, "expected sbi.value"); + else + report(false, "expected sbi.value: %ld - Got: %ld", + expected, ret->value); } static void check_base(void)