Skip to content
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

Test that we are acting as paranoid not it's behaviour #113

Merged
merged 1 commit into from
Oct 4, 2017

Conversation

tvdeyen
Copy link
Member

@tvdeyen tvdeyen commented Oct 4, 2017

We use acts_as_paranoid to enable soft delete. The current spec
for destroy tests the behaviour of the acts_as_paranoid gem.

Also it tests behaviour that is not possible with latest version of
Solidus (86d0e6281cf04a856766f7da1506d8007164f0ae) any more.
We can't destroy users that have orders assigned to them.

This is sufficienlty tested in Solidus core and must not be tested here again.
Therefore I think it is safe to remove this spec and replace it with a basic
duck typing test to ensure we are still using acts_as_paranoid.

We use `acts_as_paranoid` to enable soft delete. The current spec
for `destroy` tests the behaviour of the `acts_as_paranoid` gem.

Also it tests behaviour that is not possible with latest version of
Solidus (86d0e6281cf04a856766f7da1506d8007164f0ae) any more.
We can't destroy users that have orders assigned to them.

This is sufficienlty tested in Solidus core and must not be tested here again.
Therefore I think it is safe to remove this spec and replace it with a basic
duck typing test to ensure we are still using `acts_as_paranoid`.
@tvdeyen tvdeyen requested a review from stewart October 4, 2017 18:51
Copy link
Contributor

@stewart stewart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thanks so much @tvdeyen!

@tvdeyen tvdeyen merged commit 83c1425 into master Oct 4, 2017
@tvdeyen tvdeyen deleted the do-not-test-behaviour-of-acts-as-paranoid branch October 4, 2017 19:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants