search

approvals / ApprovalTests.cpp

Native ApprovalTests for C++ on Linux, Mac and Windows
https://approvaltestscpp.readthedocs.io/en/latest/
Apache License 2.0
318 stars 51 forks source link

Move only disposer #93

Closed p-podsiadly closed 4 years ago

p-podsiadly commented 4 years ago

Description

I came across this problem when writing an utility method, which registers the same comparator for multiple file extensions: link.

In linked code, Disposer class wraps a vector of ComparatorDisposer objects. Each ComparatorDisposer is created by FileApprover::registerComparatorForExtension, copied to a vector and destructed and at that point the comparator is immediately disposed.

Expected behavior is that the comparator is disposed once the copy in the vector is destructed. Destructor of the original ComparatorDisposer instance should not dispose the comparator.

The solution

I've added a flag isActive, which is initially set to true. The flag is set to false when a ComparatorDisposer is moved-from, to indicate that it's destructor should not restore previousComparator.

Additionally, I've declared copy constructor as deleted.

claremacrae commented 4 years ago

Hello, thank you very much for this, @p-podsiadly - I looked at the linked code and it was very exciting to see an image comparison implementation, especially one that doesn’t depend on Qt.

Would you be able to write a test for the change, that shows how it was broken, and the new behaviour, please?

I’d be worried otherwise that we might accidentally break it in future.

p-podsiadly commented 4 years ago

Hello, @claremacrae, I've added the test - fails, as expected, without the fix.

I'm not sure if the commit message should start with "F!!" or "B!!" according to the convention you're using - should I change it?

claremacrae commented 4 years ago

Hello @p-podsiadly Can you spare a few minutes to pair on this - it'll be a lot more quicker than going back and forwards here, I feel...

claremacrae commented 4 years ago

Hello @p-podsiadly Can you spare a few minutes to pair on this - it'll be a lot more quicker than going back and forwards here, I feel...

I was struggling to work out how the test works, but I think I've got it now - so I'll accept the PR then add some comments to the test, and ask you to confirm if I've understood it.

Thank you very much indeed for doing this - I think I now need to work out how to effectively apply similar changes to all the other disposers.

claremacrae commented 4 years ago

I'm not sure if the commit message should start with "F!!" or "B!!" according to the convention you're using - should I change it?

I'm happy for it to stay as F!! and count it as a feature!

p-podsiadly commented 4 years ago

I was struggling to work out how the test works, but I think I've got it now - so I'll accept the PR then add some comments to the test, and ask you to confirm if I've understood it.

If you'd like to, I'll be able to talk later today.

I guess that the "foo"/"bar" comparisons in TestComparator could be confusing. Maybe it will be better if I changed this to case-insensitive comparison and rename TestComparator to CaseInsensitiveComparator?

PS I've added another test case to check if ComparatorDisposer is not copyable.

p-podsiadly commented 4 years ago

Sorry, I've clicked "Close and comment" instead of "Comment" by mistake.

claremacrae commented 4 years ago

Sorry, I've clicked "Close and comment" instead of "Comment" by mistake.

Been there, done that - it's easily done! 😀

If you'd like to, I'll be able to talk later today.

OK, thanks. For now I'll pop a few comments in code review, to get your thoughts on them, if you have time...

claremacrae commented 4 years ago

I guess that the "foo"/"bar" comparisons in TestComparator could be confusing. Maybe it will be better if I changed this to case-insensitive comparison and rename TestComparator to CaseInsensitiveComparator?

Yes, that's a good point - for me, it did take a little while to work out the foo/bar thing... in the end, I did understand it - I've made a suggestion for an alternative approach in the code review...

p-podsiadly commented 4 years ago

I've changed the test to use FileApprover::verify, the way it is done FileApproverTest.cpp - didn't think this earlier. The test should be more self-explanatory now.

claremacrae commented 4 years ago

I've changed the test to use FileApprover::verify, the way it is done FileApproverTest.cpp - didn't think this earlier. The test should be more self-explanatory now.

Yes, it's really clear now. Thanks!

Nice idea to not store the approved file in git. That should allow cloning in Windows fine now.

~I wonder what will happen when the tests are run in Windows? I suspect it would exceed the file-name limit writing out the files and make the tests always fail.~

claremacrae commented 4 years ago

I wonder what will happen when the tests are run in Windows? I suspect it would exceed the file-name limit writing out the files and make the tests always fail.

No - I'm wrong! Because you've supplied the names a.txt and b.txt, the test name is ignored and it's all good 🎉 !

claremacrae commented 4 years ago

@p-podsiadly - this is great - I'm really happy to go ahead and merge it, if you are ready for me to do so?

I learned a lot from reading this code (in Catch and std C++) - thank you very much indeed!

p-podsiadly commented 4 years ago

I think it's ready for merging. Glad I could help :)

claremacrae commented 4 years ago

It's merged now - Thanks again!