exercism / euphoria

Exercism exercises in Euphoria.
https://exercism.org/tracks/euphoria
MIT License
3 stars 7 forks source link

poor messages #75

Open petelomax opened 3 months ago

petelomax commented 3 months ago

Noticed on the game of life task: Failed tests give incomplete messages, eg "failed: dead cells with three live neighbors become alive, expected: {" Compilation and runtime errors don't give line numbers either.

(I'm actually not quite sure anymore, but I think those were from running it online.)

axtens commented 3 months ago

Something you might want to discuss with @ErikSchierboom as to the best way to report errors

ErikSchierboom commented 3 months ago

I don't think I'm necessarily the expert here :) What you want is for the student to be able to quickly figure out what is wrong with their code. So line numbers, bits of their code and error messages all help with that. As to what's possible in Euphoria, I don't know.

petelomax commented 3 months ago

Running game of life in the CLI (compiler error):

F:\misc\exercism\euphoria\game-of-life>eutest

interpreting t_game-of-life.e:
F:\misc\exercism\euphoria\game-of-life\game-of-life.ex:7
<0132>:: Syntax error - expected to see possibly ']', not ','
      integer mxy = m[x,y], nxy = -mxy
                       ^

FAILURE: t_game-of-life.e EUPHORIA error with status 1

Test results summary:
    FAIL: t_game-of-life.e
Files (run: 1) (failed: 1) (0% success)

Running same online:

We received the following error when we ran your code:
<0132>:: Syntax error - expected to see possibly ']', not ','

Running game of life in the CLI (incorrect output):

  failed: live cells with zero live neighbors die, expected: {
  {0,0,0},
  {0,0,0},
  {0,0,0}
} but got: {
  {0,0,0},
  {0,1,0},
  {0,0,0}
}

Same online:

failed: live cells with zero live neighbors die, expected: {

So the question is what needs changing to get the extra info to show up online?

ErikSchierboom commented 3 months ago

This is the relevant code: https://github.com/exercism/euphoria-test-runner/blob/main/bin/run.ex

petelomax commented 3 months ago

@axtens can I leave that in your capable hands?

axtens commented 3 months ago

Yep. You can. It is a thing and worth the think

petelomax commented 3 months ago

I also think you'd get friendlier messages from t_space-age.e if you did it like this instead

procedure is_close(sequence desc, atom val1, atom val2)
    val1 = floor(val1*100)/100
    test_equal(desc,val1,val2)
end procedure
is_close("age on Earth", ageOn("Earth", 1000000000), 31.68)
is_close("age on Mercury", ageOn("Mercury", 2134835688), 280.87)
is_close("age on Venus", ageOn("Venus", 189839836), 9.77)
is_close("age on Mars", ageOn("Mars", 2129871239), 35.88)
is_close("age on Jupiter", ageOn("Jupiter", 901876382), 2.40)
is_close("age on Saturn", ageOn("Saturn", 2000000000), 2.15)
is_close("age on Uranus", ageOn("Uranus", 1210123456), 0.45)
is_close("age on Neptune", ageOn("Neptune", 1821023456), 0.35)
test_false("invalid planet causes error", ageOn("Sun", 680804807))

However I'm not at all certain whether truncating (as above) or rounding (as current) would be best, and suspect "the Phix way" is subtly incompatible with Euphoria, at least the way I'd naturally do it...

axtens commented 3 months ago

I'm quite okay with you forking the repo and raising a PR. I'm not sure who authored our current one, but it wasn't me. Might have been @glennj or possibly @BNAndras

BNAndras commented 3 months ago

I can make that fix later today.

BNAndras commented 3 months ago

Upon review, The problem I have with this approach is that some of the expected values will deviate slightly from the canonical ones. The deviation is occurring because of an implementation detail. Generally, the languages I've worked with had a way to compare floating point values with a specified tolerance. That'd be a more robust way of approaching this if we could suggest changes to the unit test library.

axtens commented 3 months ago

Not use eutest?? Hmm. Maybe we could do some shims. @petelomax is good at that. Can you unpack your suggestion a bit further?

petelomax commented 3 months ago

I did in fact change the Phix test_equal() to allow the default tolerance of 1e-9 to be overidden. While making a similar change to Euphoria might not be technically difficult, getting a new version shipped with that change in it would be, as things stand..

I suspect though what you really have issues with here is the val1 = floor(val1*100)/100 line above, my intention was you could replace that with round() or even a couple of sprintf(), and for that matter using the existing test but modifying the description with say desc = sprintf("%s: got:%.2f, expected:%.2f",{desc,val1,val2}) and still using a test_true() inner call should be fine.

axtens commented 3 months ago

We have two separate issues in this thread: the precision issue in space-age and the modification of the test-runner to better reflect where an error was encountered. Seeing as right here is https://github.com/exercism/euphoria/issues/75 (to which I've been assigned), how about the rest of you create a separate issue, put a link to it here, and go over there and discuss it.