Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

suggested: make test displays (expected; actual) values for failed tests #185

Merged
merged 6 commits into from
Jan 2, 2025

Conversation

guberathome
Copy link
Contributor

@guberathome guberathome commented Dec 30, 2024

It would greatly help debugging if a failed test case (e.g. in make test) would return the offending value.

Here's an example from the current MSYS2-Cygwin environment
(fixing RESIZE-FILE on this platform is a separate topic).

OOTB we get:

cd ../../fth && ../platforms/unix/pforth_standalone -q t_file.fth
...
INCORRECT RESULT: T{ FID2 @ FILE-SIZE -> 37. 0 }T
INCORRECT RESULT: T{ CBUF BUF 100 FID2 @ READ-FILE -> 37 0 }T
INCORRECT RESULT: T{ PAD 38 BUF 38 S= -> FALSE }T
...
  91 passed,    3 failed.

The suggested change would give us:

cd ../../fth && ../platforms/unix/pforth_standalone -q t_file.fth
...
INCORRECT RESULT (expected=37, got=87): T{ FID2 @ FILE-SIZE -> 37. 0 }T
INCORRECT RESULT (expected=37, got=87): T{ CBUF BUF 100 FID2 @ READ-FILE -> 37 0 }T
INCORRECT RESULT (expected=0, got=-1): T{ PAD 38 BUF 38 S= -> FALSE }T
...
  91 passed,    3 failed.

I do have a few concerns about int>str, concat and concat+free though:

  • Are similar words already defined elsewhere perhaps?
  • Is this the right place to define them (instead of e.g. t_strings.fth or strings.fth)?
    For now I preferred NOT to create a dependancy to other forth code files.

Feedback is welcome.

Copy link
Owner

@philburk philburk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I think printing the EXPECTED and ACTUAL will make debugging much easier.

fth/t_tools.fth Outdated Show resolved Hide resolved
fth/t_tools.fth Outdated
-1 test-passed +!
1 test-failed +!
S" INCORRECT RESULT: " error
s" INCORRECT RESULT (expected=" R> int>str concat
s" , got=" concat+free R> int>str concat+free
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There may be a memory leak here because ERROR does not free the memory allocated by CONCAT.
Rather that trying to insert the new message into the ERROR message, let's just add this after the call to ERROR.

Suggested change
s" , got=" concat+free R> int>str concat+free
." (expected=" r> .
." , got=" r> .
." )"

Then we don't need the INT>STR or CONCAT tools.

Copy link
Contributor Author

@guberathome guberathome Dec 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, not having to deal with memory (de)allocation would be a definite bonus.

However, having the values at the start of each line makes it easier to read for me.
Could we perhaps directly output the values BEFORE calling error?

            if
                2drop
            else
                -1 test-passed +!
                1 test-failed +!
                ." INCORRECT RESULT (got=" (.) type
                ." , expected=" (.) type
                s" ) from "
                error
                LEAVE
            THEN

This would give as output:

cd ../../fth && ../platforms/unix/pforth_standalone -q t_file.fth
...
INCORRECT RESULT (got=87, expected=37) from T{ FID2 @ FILE-SIZE -> 37. 0 }T
INCORRECT RESULT (got=87, expected=37) from T{ CBUF BUF 100 FID2 @ READ-FILE -> 37 0 }T
INCORRECT RESULT (got=-1, expected=0) from T{ PAD 38 BUF 38 S= -> FALSE }T
...
  91 passed,    3 failed.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that the actual/expected looks better where you put it.
But I don't like passing a funky ") from" to the ERROR word.
This was inherited from the test framework. We can change it but I am planning to update to a more official
version of the test framework. So we will have to carry over these changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Output is now:

(got=87, expected=37) INCORRECT RESULT: T{ FID2 @ FILE-SIZE -> 37. 0 }T
(got=87, expected=37) INCORRECT RESULT: T{ CBUF BUF 100 FID2 @ READ-FILE -> 37 0 }T
(got=-1, expected=0) INCORRECT RESULT: T{ PAD 38 BUF 38 S= -> FALSE }T

@guberathome guberathome requested a review from philburk January 1, 2025 00:11
fth/t_tools.fth Outdated Show resolved Hide resolved
Co-authored-by: Phil Burk <philburk@mobileer.com>
@guberathome guberathome marked this pull request as draft January 1, 2025 09:16
@guberathome guberathome marked this pull request as ready for review January 1, 2025 11:33
@guberathome guberathome requested a review from philburk January 1, 2025 11:34
Copy link
Owner

@philburk philburk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice. A small simple change that helps debugging.

@philburk philburk merged commit 818e8e9 into philburk:master Jan 2, 2025
3 checks passed
@guberathome guberathome deleted the question-241230 branch January 2, 2025 06:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants