Show code at error location in expect-style tests by gasche · Pull Request #1511 · ocaml/ocaml

gasche · 2017-12-03T19:30:11Z

Old behavior (lame):

type unknown += Foo;;
(* unknown, not the whole line *)
[%%expect{|
Line _, characters 5-12:
Error: Unbound type constructor unknown
|}];;

new behavior (so awesome):

type unknown += Foo;;
(* unknown, not the whole line *)
[%%expect{|
Line _, characters 5-12:
  type unknown += Foo;;
       ^^^^^^^
Error: Unbound type constructor unknown
|}];;

v-gb · 2017-12-03T20:38:28Z

testsuite/tests/typing-modules/aliases.ml

 module F : functor (X : sig  end) -> sig type t end
 Line _:
+  module C = Char;;
+


This module C = Char is the first line of the file, because I suppose there is no location and so line is 1 and I am not sure about the characters. It would be preferable to print nothing than to print the first line.

Ideally, the expectation would be clearer about the complete lack of position, so developers are more likely to fix them and reviewers more likely to point them out.

Oh, the characters are probably 0-0, which is why nothing is underlined.

Actually, I think that grepping for Line _: is enough to detect fishy locations: the characters are not printed when they look non-sensical, and I think this coincides with what you want to detect. In that case, I would suggest not changing anything in the PR specifically: (1) it's an orthogonal concern and (2) the reasonable need that you suggested can already be satisfied in the current state (if my guess is right that Line _: coincides with the situation where you think we should "be clearer").

Oh ok, this is the one place where something fishy is going on.
It's actually more fishy than I first thought, because ocaml/expect_test give one error, and ocamlc/ocamlopt give a different looking error, which I think is a better version of the same error, with a position in particular.

In any case, it still seems to me that the whole show_code_at_location call should be below a if startchar >= 0, to avoid printing unrelated code.

expect_test is using Toploop which is the ocaml (toplevel) path, maybe the discrepancy can be reproduced there? You are right that it's interesting enough to investigate, I'll try to have a look.

v-gb · 2017-12-03T20:43:37Z

testsuite/tools/expect_test.ml

+    | Some lexbuf ->
+        Location.show_code_at_location ppf lexbuf loc
+    end;
+    ()


Wouldn't it be plain simpler to call the existing highlight_dumb function without the ~print_chars flag, which would change the output from:

Line _, characters 1-2: text ^^

to:

Characters 1-2: text ^^

So we tried that first, and the result is something like

Characters 346-373:

and it is not stable to re-running the script again with -principal filled in, so it doesn't actually work.

Ok, expect_tests works on the whole file, so positions are from the beginning of the file, whereas the toplevel works phrase by phrase. Fair enough.

v-gb · 2017-12-03T20:51:47Z

Presumably the goal is to be able to review the location of errors more effectively? Looks good to me.

You apparently missed testsuite/tests/typing-misc/pr6939.ml-{flat,noflat} in your rewrite.

gasche · 2017-12-04T00:15:58Z

Presumably the goal is to be able to review the location of errors more effectively? Looks good to me.

Yes, thanks!

You apparently missed testsuite/tests/typing-misc/pr6939.ml-{flat,noflat} in your rewrite.

Indeed. I fixed the -flat one and rebased, but -noflat seems to be actually failing in trunk, I'll have to investigate more -- see #1471 (comment).

gasche · 2017-12-04T00:39:25Z

@sliquister have you reviewed the whole PR in enough details that you would formally "Approve" of it?

v-gb

I didn't look at every single test in details, but what I looked at was good. I think you should make the one line change to avoid printing code when the position has negative characters before merging, though.

gasche · 2017-12-10T16:37:29Z

I just performed the change requested by @sliquister. Thanks!

damiendoligez

I have reviewed the compiler changes and they look good. I didn't look at all the tests but I don't think they need to be carefully reviewed.

Not merged because @gasche might want to investigate the appveyor failure.

gasche · 2017-12-12T14:27:14Z

@damiendoligez thanks a lot for the additional review.

I cannot make sense of the AppVeyor failure: the log appears to be cut (both in the web view and in the raw log) in the middle of compilation, it hasn't reached the testsuite yet. @dra27, would you by chance have an idea of what is happening here?

(Is there a way to restart an AppVeyor build? I could close-reopen the issue, but that would also restart the Travis build.)

I will have to fix the conflicts in any case, so I will push again, which may make the issue disappear.

This uses the "highligh_dumb" machinery, but is explicitly designed to not print the location itself. This is for use in the expect-style tests; the expect-test modification will come in a separate commit, but it looks as nice as the following: [%%expect{| Line _, characters 1-4: #bar) -> ();; ^^^ Error: Unbound class bar |}];;

(joint work with Armaël Guéneau)

* Industrial-users: update xenserver entry * Jobs: drop xenserver entry --------- Signed-off-by: Edwin Török <edwin.torok@cloud.com>

gasche changed the title ~~Expect highlight locations~~ Show code at error location in expect-style tests Dec 3, 2017

v-gb reviewed Dec 3, 2017

View reviewed changes

v-gb approved these changes Dec 4, 2017

View reviewed changes

gasche force-pushed the expect-highlight-locations branch from 49f2d41 to 5ee39bf Compare December 10, 2017 16:36

damiendoligez approved these changes Dec 11, 2017

View reviewed changes

gasche added 3 commits December 12, 2017 15:35

expect-style tests: include the code at the location of error messages

91dff32

(joint work with Armaël Guéneau)

Changes entry

a607486

gasche force-pushed the expect-highlight-locations branch from 5ee39bf to a607486 Compare December 12, 2017 14:53

gasche merged commit a923eb4 into ocaml:trunk Dec 13, 2017

gasche mentioned this pull request Dec 13, 2017

Migrate expect tests to ocamltest #1519

Merged

Conversation

gasche commented Dec 3, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

v-gb commented Dec 3, 2017

Uh oh!

gasche commented Dec 4, 2017

Uh oh!

gasche commented Dec 4, 2017

Uh oh!

v-gb left a comment

Choose a reason for hiding this comment

Uh oh!

gasche commented Dec 10, 2017

Uh oh!

damiendoligez left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gasche commented Dec 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

damiendoligez left a comment •

edited

Loading

gasche commented Dec 12, 2017 •

edited

Loading