When applying inline snapshot, generate prefix/suffix using char offsets

[c-spencer]

Fixes #137

Previously the columns delimiting the replacement were being interpreted as byte offsets into their lines, rather than char offsets. This fix just updates the prefix/suffix construction to use a chars iterator.

I wasn't sure on how/where to add tests for this, as this code is only run as part of cargo-insta, and the existing tests seem to only check the snapshot generation/equivalency, rather than inline application.

Supporting are two action runs, before and after this change:

• before: https://github.com/c-spencer/insta-unicode-repro/actions/runs/351125741
• after: https://github.com/c-spencer/insta-unicode-repro/actions/runs/351158192

I've also tested locally on that repository and the patch is applied correctly (i.e. without duplicated parts of the previous snapshot in the suffix, as was happening for me and as described in #42 previously).

[max-sixty]

This looks good. The start and end come from LineColumn, which is indeed in UTF-8 characters rather than bytes.

Still not sure why I couldn't reproduce though!

[max-sixty]

One more note — the existing code doesn't seem to run the assert_snapshot!("a 😀oeu", @"a 😀oeu"); test correctly — try removing the expected value and the existing version of insta will put it back in the wrong place.

I'm not sure how we ended up here — this PR solves the problems but is basically a reversion of https://github.com/mitsuhiko/insta/pull/43/files. As I wrote a couple of years ago now — would be great to find a way of testing this code better. Very open to ideas on how to do that and happy to execute.

An aside — @c-spencer — I figured out why I couldn't replicate — I was using a different machine which had an older version of insta on. Mea culpa.

[c-spencer]

Aha, maybe the way the offsets were collected was changed (and with no tests, not caught)?

Initial inclination would be something along the lines of tests in cargo-insta driving it using internals, against expected output. Quick look at inline.rs it seems it should be relatively easy to construct a FilePatcher manually and assert on content of lines.

Alternatively an outside-in approach would be to just have a before.rs/after.rs and a test script to drive the CLI.

[mitsuhiko]

I'm sufficiently confused now that I will need to look into it. Maybe we were confused by "utf-8 character". Normally you would assume this to be defined as "Unicode Character" (char) and "UTF-8 Offset" (bytes).

[max-sixty]

I'm sufficiently confused now that I will need to look into it. Maybe we were confused by "utf-8 character". Normally you would assume this to be defined as "Unicode Character" (char) and "UTF-8 Offset" (bytes).

While I haven't looked into it enough — I can if you want me to — I'm fairly confident that @c-spencer 's suggestion is correct — that the code was correct 18 months ago, then something other than these lines changed, and now it's incorrect. So reverting these lines makes it work again.

What are your thoughts on testing this code @mitsuhiko ? That seems like the underlying problem here. I haven't written much rust recently so not sure what the latest & greatest is for these sorts of high context tests.

[mitsuhiko]

@max-sixty i will look into it now.

[mitsuhiko]

Thanks. I will add tests now.

[max-sixty]

Thanks @mitsuhiko !

FYI I've got a bit more time now so happy to take some of this on again. Though the crate is somewhat feature-complete at this stage.

Tangentially — I had also tried doing inline snapshot tests for python, but decided it's probably not possible with standard asserts: https://github.com/max-sixty/pytest-accept