Isolation of components during testing #1296

Closed
wants to merge 40 commits into
from

Conversation

Projects
None yet
5 participants
@radar
Member

radar commented Mar 20, 2012

Hi,

There was an issue (#1292) brought up yesterday where Spencer attempted to run tests for an extension only to run into an issue where we were referencing current_user inside of Core when we should truly never be doing something like that. All references to current_user should be in the auth component.

To ensure that something like this never comes up again, I've taken the time to separate the testing of the individual components of Spree and have run the tests to determine if there were any more places where issues like #1292 would come up. There were, and they have been fixed.

When the build passes for the isolation branch on Travis, it should be good to merge this in... providing that everyone agrees that this is the way forward.

radar added some commits Mar 19, 2012

Don't use a Factory inside admin/base_controller_spec
This is because, due to isolation, there are no roles for users inside of Core
Never stub current_user inside core's controller specs
Why? Well, because Core never has a concept of current_user. This only needed to be stubbed because auth was inadvertently being included when we were running the tests
load_roles in admin_users_controller_decorator should return a scoped…
… object, so no database call is done until necessary
Do not sign in as an admin during any core request spec
This is because core has no concept of authentication/authorization.
product attributes in product_prototype should not attempt to create …
…a tax_category or shipping_category object.

This is because most product tests don't need these objects.

By avoiding creating them, the tests that use these attributes should be faster
The flow of creating a product is well tested in request specs. No po…
…int also having a controller spec for it in admin/products_controller_spec
Each component will now have its own Gemfile.
This is to address the problem in #1292.
Remove all_tests rake task, go into each directory one by one and run…
… the tests.

This is because otherwise the libraries that Bundler load tend to leak. So what we want to do is go into every single directory and run the tests JUST for that component. One by one.
Define set_gemfile alias to set BUNDLE_GEMFILE before each component …
…runs.

This is because Travis automatically sets this to be the Gemfile at the beginning of the test run which would cause all components to be used. What we want instead is for each component to use just its own Gemfile.
@schof

This comment has been minimized.

Show comment
Hide comment
@schof

schof Mar 20, 2012

Member

@radar major objection to this. lets talk about this later today

Member

schof commented on 134f2f3 Mar 20, 2012

@radar major objection to this. lets talk about this later today

This comment has been minimized.

Show comment
Hide comment
@radar

radar Mar 20, 2012

Member

I think it's important as it allows all components to be tested in isolation. If people are wanting to only use core (as shown in #1292) then we should ensure that they can do that.

Member

radar replied Mar 20, 2012

I think it's important as it allows all components to be tested in isolation. If people are wanting to only use core (as shown in #1292) then we should ensure that they can do that.

This comment has been minimized.

Show comment
Hide comment
@citrus

citrus Mar 20, 2012

Contributor

@schof - @radar is right. We need to test each part of spree in isolation to ensure that the modularity of spree stays in tact. v1.0.2 and v1.0.3 both made the core useless without spree_auth.

Contributor

citrus replied Mar 20, 2012

@schof - @radar is right. We need to test each part of spree in isolation to ensure that the modularity of spree stays in tact. v1.0.2 and v1.0.3 both made the core useless without spree_auth.

radar added some commits Mar 20, 2012

Move Spree::Gateway::Test to a spec/support file so it can be loaded …
…by both spec/models/payment_method_spec and spec/requests/admin/payment_method
Stub Order's require_email to be false for shipping method tests
This is because when the test goes through the checkout process it fails on the address page due to lack of email. We *could* just fill out the email field, but there simply isn't one in this case... therefore we do this small hack.
@radar

This comment has been minimized.

Show comment
Hide comment
@radar

radar Mar 20, 2012

Member

Build is all fixed now: http://travis-ci.org/#!/spree/spree/builds/908921. This can be merged into master.

Member

radar commented Mar 20, 2012

Build is all fixed now: http://travis-ci.org/#!/spree/spree/builds/908921. This can be merged into master.

@citrus

This comment has been minimized.

Show comment
Hide comment
@citrus

citrus Mar 21, 2012

Contributor

You're the man Ryan! 🍺 🍺

Contributor

citrus commented Mar 21, 2012

You're the man Ryan! 🍺 🍺

@radar

This comment has been minimized.

Show comment
Hide comment
@radar

radar Mar 21, 2012

Member

Based on a discussion I had with Sean yesterday, these changes probably won't make it into 1-0-stable, but will be applied directly to master -- and therefore contribute to the 1-1-stable release. A patch release, in our opinion, should not introduce so many large changes like this.

The patch cmar gave to fix the issue will be released in 1.0.4, meaning your extensions tests should work on that version. We'll be making sure of that before we do a release.

We'll be releasing 1-1-stable later this week most likely.

Member

radar commented Mar 21, 2012

Based on a discussion I had with Sean yesterday, these changes probably won't make it into 1-0-stable, but will be applied directly to master -- and therefore contribute to the 1-1-stable release. A patch release, in our opinion, should not introduce so many large changes like this.

The patch cmar gave to fix the issue will be released in 1.0.4, meaning your extensions tests should work on that version. We'll be making sure of that before we do a release.

We'll be releasing 1-1-stable later this week most likely.

radar added a commit that referenced this pull request Mar 21, 2012

Isolate each component of Spree
This commit contains a whole bunch of changes, from pull request #1296
which will fix #1292.

To understand the gist of the problem, please see the comment threads in

The main bunch of work that was done here was done in two parts: remove all current_user
references from Core and to isolate each component during its testing so
that it is not loading all of Spree for each component, just the
bare-bones components that are needed.

For instance, Core now will only load Core. API will now load API, Auth
and Core. Dash will load Auth and Core. Promo will load Auth and Core.
@radar

This comment has been minimized.

Show comment
Hide comment
@radar

radar Mar 21, 2012

Member

Merged in @6dce9ba8b25c3a577255f08be37a2d9bc2a30b99.

Member

radar commented Mar 21, 2012

Merged in @6dce9ba8b25c3a577255f08be37a2d9bc2a30b99.

@radar radar closed this Mar 21, 2012

@citrus

This comment has been minimized.

Show comment
Hide comment
@citrus

citrus Mar 21, 2012

Contributor

Thanks so much Ryan, I'm looking forward to the 1.0.4 and 1.1.0 releases!

Contributor

citrus commented Mar 21, 2012

Thanks so much Ryan, I'm looking forward to the 1.0.4 and 1.1.0 releases!

tvdeyen pushed a commit to magiclabs/spree that referenced this pull request Apr 5, 2012

Isolate each component of Spree
This commit contains a whole bunch of changes, from pull request #1296
which will fix #1292.

To understand the gist of the problem, please see the comment threads in

The main bunch of work that was done here was done in two parts: remove all current_user
references from Core and to isolate each component during its testing so
that it is not loading all of Spree for each component, just the
bare-bones components that are needed.

For instance, Core now will only load Core. API will now load API, Auth
and Core. Dash will load Auth and Core. Promo will load Auth and Core.
@sbounmy

This comment has been minimized.

Show comment
Hide comment
@sbounmy

sbounmy May 1, 2012

Contributor

hi @radar,

Is this issue fixed in spree_core 1.0.4 ? my extension's spec are failling when updating to 1.0.4

Contributor

sbounmy commented May 1, 2012

hi @radar,

Is this issue fixed in spree_core 1.0.4 ? my extension's spec are failling when updating to 1.0.4

@citrus

This comment has been minimized.

Show comment
Hide comment
@citrus

citrus May 2, 2012

Contributor

I'm still seeing broken tests on 1.0.4 as well...

Contributor

citrus commented May 2, 2012

I'm still seeing broken tests on 1.0.4 as well...

@radar

This comment has been minimized.

Show comment
Hide comment
@radar

radar May 2, 2012

Member

This would've been nice to know before we released :\ Why weren't you testing on 1-0-stable? Could you please provide more information about the broken tests you are encountering?

Member

radar commented May 2, 2012

This would've been nice to know before we released :\ Why weren't you testing on 1-0-stable? Could you please provide more information about the broken tests you are encountering?

@citrus

This comment has been minimized.

Show comment
Hide comment
@citrus

citrus May 2, 2012

Contributor

The issue I'm still seeing is issue #1292 (current_user method error)

I'll test against the 1-0-stable branch in the future, but it would be nice if the spree team tested new versions against some popular extensions before they are released as well.

To further the efforts of making Spree releases more stable, we would love to have a collection of extensions outside of the Spree development team that we can test to ensure stability. Do you mind if we include spree_essentials in that list? We'll be testing these extensions before we release another 1.0.x release, as well as all future releases of any Spree branch.

(#1292 (comment))

On another note, 1.1.0 is working great without any breakage. Thanks!!

Contributor

citrus commented May 2, 2012

The issue I'm still seeing is issue #1292 (current_user method error)

I'll test against the 1-0-stable branch in the future, but it would be nice if the spree team tested new versions against some popular extensions before they are released as well.

To further the efforts of making Spree releases more stable, we would love to have a collection of extensions outside of the Spree development team that we can test to ensure stability. Do you mind if we include spree_essentials in that list? We'll be testing these extensions before we release another 1.0.x release, as well as all future releases of any Spree branch.

(#1292 (comment))

On another note, 1.1.0 is working great without any breakage. Thanks!!

@sbounmy

This comment has been minimized.

Show comment
Hide comment
@sbounmy

sbounmy May 2, 2012

Contributor

@radar well, you said

The patch cmar gave to fix the issue will be released in 1.0.4, meaning your extensions tests should work on that version. We'll be making sure of that before we do a release.

So I was basically waiting for 1.0.4 release..I will also test again 1-0-stable in the future.

Contributor

sbounmy commented May 2, 2012

@radar well, you said

The patch cmar gave to fix the issue will be released in 1.0.4, meaning your extensions tests should work on that version. We'll be making sure of that before we do a release.

So I was basically waiting for 1.0.4 release..I will also test again 1-0-stable in the future.

@ygor

This comment has been minimized.

Show comment
Hide comment
@ygor

ygor Jun 11, 2012

So is this fixed in 1.0.4 ?

ygor commented Jun 11, 2012

So is this fixed in 1.0.4 ?

@radar

This comment has been minimized.

Show comment
Hide comment
@radar

radar Jun 11, 2012

Member

@ygor: Do you have reason to think that it's not?

Member

radar commented Jun 11, 2012

@ygor: Do you have reason to think that it's not?

@ygor

This comment has been minimized.

Show comment
Hide comment
@ygor

ygor Jun 11, 2012

Well, I'm trying to run the tests on spree_essential_cms and spree_essential_blog and get the missing current_user method error, which was exactly the cause for this fix..

ygor commented Jun 11, 2012

Well, I'm trying to run the tests on spree_essential_cms and spree_essential_blog and get the missing current_user method error, which was exactly the cause for this fix..

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