Don't allow missing snapshots without --snapshot-update

[davidshepherd7]

Fixes #73

Possibly should only be included in a new major version so that it doesn't break people's CI.

[dashmug]

Why not add a command line flag to enable it (disabled by default) so you can ship this in a minor version?

Then, you can have that flag enabled by default in the next major version.

[davidshepherd7]

Yeah, I suspect something like that is the way to do it. Unfortunately this repository seems to be unmaintained.

[paulmelnikow]

Hi, thanks for this! I just got write access to the repo. I left you a comment.

[paulmelnikow]

If I call assert_match(..., update=False) I would expect no updates to happen, but if self.update is set that would take precedence. This is a bit confusing.

What is the purpose of this parameter? Is it intended only for testing?

If so, I wonder if it would be better for tests to manipulate the .update property, rather than add a parameter to this function.

[davidshepherd7]

I wrote this a while ago, but yeah I think it's only used in tests. I think I did it this way because adding self.update = True and self.update = False around every test is a lot of boilerplate (it's as many lines of boilerplate as are in most of the tests in the first place).

I definitely agree that it's confusing though. For now I'll rename it to _force_update_snapshots, if you think that's ok then you can merge, or if not we can try to think of something else.

[paulmelnikow]

I think I did it this way because adding self.update = True and self.update = False around every test is a lot of boilerplate (it's as many lines of boilerplate as are in most of the tests in the first place).

Does it need to be set back to False? I wouldn't think so. Though if it does, what about using a context handler?

[davidshepherd7]

I had another look: the reason I didn't do it that way was actually because update was a getter function and had some interactions with different testing frameworks that I didn't really understand.

I've pushed a commit with a refactoring that turns it into a plain member variable, and then sets that variable. I think the code is cleaner but I'm less confident that it will work correctly with all supported testing frameworks because I'm not sure I fully understand what the code was doing before (and I don't have anywhere to test django or nose other than the unit tests).

Up to you whether we merge with or without the refactor.

[paulmelnikow]

I think we probably need to rely on the unit tests, or improve them if they aren't covering the behavior they need to.

What do you imagine might break?

[davidshepherd7]

I'm worried about "unknown unknowns" because I don't know what I need to worry about with those frameworks (in my experience test frameworks sometimes have magical behaviour, so things can look like they should work but not work).

I think it's probably fine, but maybe we can do an rc or beta build first and get people who are using those frameworks to try it out? Or maybe you're right and we should just trust the unit tests.

[paulmelnikow]

Sounds good. Let's plan on shipping a bunch of betas until we have tackled all the breaking changes in a release that we've given the community at least a couple weeks to validate.

[davidshepherd7]

@paulmelnikow Do you think it's worth adding a command line flag to control this behaviour?

I feel like this should be the default behaviour ASAP because the old version could cause spurious test passes. So I think it would be reasonable to release a new major version now-ish with no breaking changes other than this. If we do that then I think there's no need to add a flag and immediately deprecate it because anyone who wants to opt-in to the new behaviour can upgrade and anyone who doesn't want it right now can not upgrade.

[paulmelnikow]

I feel strongly that this the current behavior is incorrect and dangerous. Until someone articulates an important use case for the flag (that can't be worked around easily) I think we should not provide a flag.

I think this is a bug fix, but agreed it is a major enough change in behavior that it should be called a breaking change.

I feel a lot of urgency about fixing this too; just want to do so with care 😀 Once we get it in good shape I think we can merge it and set the version number accordingly.

[paulmelnikow]

I'm a bit confused about why should_update_snapshot is used in some places and snapshot_should_update in others. I think this probably needs a closer look before it's merged.

[davidshepherd7]

Oh yeah, I didn't notice that there was a snapshot_should_update on some types of test case already. I've changed it to use that now.

I also rebased onto the black commit by running black on my own branch, squashing my branch, and rebasing (merging normally was a complete mess). I think it's done something sensible.

[medmunds]

(Also closes #25, I think)

[paulmelnikow]

This looks good to me, though I’d like to try it out a little bit before we merge it. @medmunds could you review, too? We also need to update the documentation.

[medmunds]

rebased onto the black commit

tests/test_formatter.py and tests/test_sorted_dict.py seem to have been reformatted without any other changes, and should probably be omitted from this PR.

Other than that, looks good to me. (And much easier to follow the should_update logic across the different frameworks now!)

[skovhus]

@paulmelnikow @davidshepherd7 it would be awesome to get this merged. Anything I can do to help here?

[davidshepherd7]

IIRC the code is all ready to go and it just needs someone to merge it.

…

On Thu, 13 May 2021, 19:44 Kenneth Skovhus, ***@***.***> wrote: @paulmelnikow <https://github.com/paulmelnikow> @davidshepherd7 <https://github.com/davidshepherd7> it would be awesome to get this merged. Anything I can do to help here? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#112 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAGCJVFDVW2M5SQAOSOGMFDTNQMX5ANCNFSM4KVX43FA> .

[skovhus]

It would make sense to merge and just release as part of the 1.0 beta.

[jackton1]

Where are we on merging this ??

Hit this issue a couple of times.

[j0nm1]

Any updates here? This is a major problem for bigger projects.