New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

3.1.0 not backward compatible with 3.0.0 #433

Closed
christianbundy opened this Issue Nov 22, 2016 · 8 comments

Comments

Projects
None yet
4 participants
@christianbundy

christianbundy commented Nov 22, 2016

Does this project use semver? It looks like there's a backward-incompatible change between 3.0.0 and 3.1.0 that ended up failing any tests that relied on mail.outbox. What versions should we be tracking to avoid backward-incompatible changes?

@pelme

This comment has been minimized.

Member

pelme commented Nov 23, 2016

Thanks for the report! In the case with mail.outbox it was never documented that it was actually cleared. We figured it was not covered by the normal backward compatibility guarantees. However we preferred to make it clear that it was not cleared and raising errors instead of silently stop cleaning it and potentially having tests pass that really should fail when we introduced the mailoutbox API.

I'm sorry it caused you trouble. I've amended the changelog to include a simple workaround to keep the old behavior until you can upgrade to the mailoutbox fixture: http://pytest-django.readthedocs.io/en/latest/changelog.html#compatibility

@pelme pelme closed this Nov 23, 2016

@ryankask

This comment has been minimized.

ryankask commented Nov 23, 2016

Thanks for providing the workaround.

I think the project should be upgraded to 4.0 because this will break many users' projects and I was surprised to see a breaking change in a point release.

Alternatively, an option should be used to opt-in to this behaviour and a warning added that this will change in the next major version.

It's true the feature was never documented, but Django's TestCase does this by default and the current behaviour, while perhaps not "pytestic", is what is expected by many users of the plugin.

@christianbundy

This comment has been minimized.

christianbundy commented Nov 23, 2016

Thanks for the quick response. I have to agree with @ryankask above, if this project uses semantic versioning (which it seems to?) I think backward-incompatible changes should bump the major version rather than the minor.

However we preferred to make it clear that it was not cleared and raising errors instead of silently stop cleaning it and potentially having tests pass that really should fail when we introduced the mailoutbox API.

I understand your point that the behavior of mail.outbox is a new feature, and I agree that not throwing an error would be silly, but if the new feature is backward-incompatible it seems that it should bump the major version rather than the minor. I get that mail.outbox wasn't explicitly part of your API, but unless the 3.0.0 API said "don't use mail.outbox, it's unreliable" I think it would be reasonable to expect 3.1.0 to continue working the same way that 3.0.0 did. It isn't that mailoutbox is hard to implement, it's that I'd like to trust that pytest-django>=3,<4 isn't going to break our already-existing code.

Thanks for your work on this project, cheers!

@pelme

This comment has been minimized.

Member

pelme commented Nov 23, 2016

Clearly this caused more problems than I anticipated. :-)

I propose that we add back the automatic clearing of mail.outbox and release 3.1.1.

Then if anyone is interested in pushing it, pytest-django 4.0 could emit a warning about mail.outbox being deprecated and in pytest-django 5.0 we can raise explicit errors. However, I think we should never remove the explicit errors since it would be very easy to accidentally screw up by relying on an unreliable outbox and having broken tests. And then, what's the point of going through all the pain of deprecating it just to end up with a more complicated solution that guards the outbox? :-)

So, let's revert to clearing the mail.outbox reliably while still keeping the mailoutbox fixture, then we can continue the discussion from there. :-)

If anyone is interested in working on a PR to bring it back that would be really great, otherwise I will try to find some time during the weekend.

/cc @peterlauri

@pelme pelme reopened this Nov 23, 2016

@peterlauri

This comment has been minimized.

Contributor

peterlauri commented Nov 23, 2016

@pelme I'll do that, if you assign it to me ;)

@peterlauri

This comment has been minimized.

Contributor

peterlauri commented Nov 23, 2016

Think this is done now in PR #434

@pelme pelme closed this in #434 Nov 24, 2016

@pelme

This comment has been minimized.

Member

pelme commented Nov 24, 2016

Thanks for the reports and the quick fix!

3.1.2 is now out which restores the old behavior.

@ryankask

This comment has been minimized.

ryankask commented Nov 24, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment