Update integration tests for Rust and error groups

[fmdkdd]

See individual commit messages for an overview.

Implementation note: Currently the group property, as used by the rustc parser is populated with make-symbol values. This makes testing these values difficult using equal. I've worked around the issue by doing the comparison on copies of errors that have group set to nil, and added a second check using flycheck-related-errors.

The better solution would be to use group values that can be checked against in the integration tests. Or somehow redefine equal to optionally disregard the group slot. I'm open to suggestions.

[cpitclaudel]

This looks very nice, thanks.

The better solution would be to use group values that can be checked against in the integration tests. Or somehow redefine equal to optionally disregard the group slot. I'm open to suggestions.

What about using consecutive numbers for groups? That is, use seq-map-indexed in -parse-rustc, and use the index as the group number in -parse-rustc-disagnostic.

[fmdkdd]

What about using consecutive numbers for groups?

I think that would work. I went for symbols first because that was the easiest way to avoid collisions. If we use consecutive numbers, then we risk collisions since flycheck-related-errors will group errors based on that value alone.

I think it should be okay for Rust currently, since we can only have one Rust checker active at at time.

[cpitclaudel]

If we use consecutive numbers, then we risk collisions since flycheck-related-errors will group errors based on that value alone.

Good point. What about using "rustc-%d"? Alternatively, should flycheck-related-errors group by group id and checker?

[fmdkdd]

Alternatively, should flycheck-related-errors group by group id and checker?

It's probably best, yes!

Also what version of seq is seq-map-indexed in?

[cpitclaudel]

Also what version of seq is seq-map-indexed in?

I see it in both seq-24 and seq on master

[fmdkdd]

Yeah I see it on master but not in seq 2.3 that we currently have a dependency for flycheck. I tried to find a changelog but didn't find any. I ask because I recall an issue around here about using a seq version other than the one bundled with Emacs 24.3, I think.

[cpitclaudel]

Indeed. I see it in seq-2.18.

[cpitclaudel]

And also in 2.12

[fmdkdd]

Ah, there is an unforeseen issue with using consecutive numbers. Cargo may call multiple compilation target in parallel, and the order of the diagnostics may not be the same at every run.

For instance, I'm seeing runs with groups 2, 2, 3, 3 and then runs with 4, 4, 5, 5. Now, in this particular case it looks like numbering the groups after error filtering would work, but... it's messy.

[cpitclaudel]

Hmm, I don't understand too well. You mean that the output of cargo-check isn't deterministic? :/

[purcell]

Awesome, thanks for picking this up!

[fmdkdd]

You mean that the output of cargo-check isn't deterministic? :/

For the same file, it will always return errors in the same order, but since cargo checks several files in parallel, the diagnostic sets of each file will interleave non-deterministically.

Since now we do not filter errors from other files, it seems predicting the group value set by the parser will be difficult.

[cpitclaudel]

OK, understood. Your approach of stripping the group ids might be the best, then. The only other approach I can think of is to use one of the errors in a group as the group identifier (think union-find).

[fmdkdd]

(think union-find).

That could be interesting, yes. I might try that.

[fmdkdd]

That could be interesting, yes. I might try that.

I tried to use the first error of a group as the value for the group property of all related errors. The main issue I'm running into is that specifying the expected errors can be tricky due to the circularity.

It's possible to group the expected errors together, to avoid spelling out the group property:

'((1 1 info "msg" :checker rust)
  (1 1 info "msg" :checker rust))
'((2 2 info "msg" :checker rust))

In flycheck-ert, we can reconstruct that by assuming the first error of the list will be used for the group property of all errors in the same list. However, this assumption does not hold for all tests (as errors are re-ordered after parsing), so we have to decorrelate the error order from the groups.

We could use the circular notation instead:

   '(1 1 info "msg" :checker rust :group #1#)
'#1=(1 1 info "msg" :checker rust :group #1#)
'#2=(2 2 info "msg" :checker rust :group #2#)

But this does not work either, since #1# must appear after #1= is defined. Even if it did parse correctly, we would need some extra juggling since these are arguments to flycheck-error-new-at, and the group property should point to the flycheck-error created by the argument list.

This leaves us with coming up with a special marker to distinguish the reprensentative error (the one used as the group value for errors in the same group).

But at this point, I feel the original solution of stripping groups was more straightforward, especially as it does not involve changing the syntax of the tests.

[cpitclaudel]

But at this point, I feel the original solution of stripping groups was more straightforward, especially as it does not involve changing the syntax of the tests.

Yes, I agree entirely.

[fmdkdd]

Yes, I agree entirely.

Please do review and merge then :)

[fmdkdd]

@cpitclaudel Thanks!