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

Introduce NullShow object #1220

Closed
wants to merge 1 commit into from
Closed

Conversation

r00k
Copy link
Contributor

@r00k r00k commented Feb 18, 2015

Previously, any test that hit the Explore page needed to create a Show
in the database with a particular name ("The Weekly Iteration").

Instead, this commit returns a NullShow if that record cannot be found.

it "returns it" do
user = double
the_weekly_iteration = double
allow(Show).to receive(:the_weekly_iteration).and_return(the_weekly_iteration)

Choose a reason for hiding this comment

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

Line is too long. [86/80]

Previously, any test that hit the Explore page needed to create a Show
in the database with a particular name ("The Weekly Iteration").

Instead, this commit returns a NullShow if that record cannot be found.
@r00k r00k force-pushed the bo-oi-decouple-weekly-iteration branch from 4a22683 to 6562686 Compare February 18, 2015 19:39
@JoelQ
Copy link
Contributor

JoelQ commented Feb 18, 2015

Does the weekly iteration ever not exist?

@r00k
Copy link
Contributor Author

r00k commented Feb 18, 2015

@JoelQ Pretty much only in tests. And dev, potentially.

@JoelQ
Copy link
Contributor

JoelQ commented Feb 18, 2015

Ah, I see. Won't you still be hitting the database, returning nil, and then returning the null object?

@r00k
Copy link
Contributor Author

r00k commented Feb 18, 2015

Yes. I don't see how we can avoid hitting the DB though. We need to check if it exists somehow.

@JoelQ
Copy link
Contributor

JoelQ commented Feb 18, 2015

Any reason not to use a stub here instead?

@r00k
Copy link
Contributor Author

r00k commented Feb 18, 2015

Then I go from needing to create a Show in every test that hits that page to needing to make a stub for any test that hits that page.

@r00k
Copy link
Contributor Author

r00k commented Feb 18, 2015

Admittedly, it does feel a little weird to make a null object just for tests.

@r00k
Copy link
Contributor Author

r00k commented Feb 18, 2015

Maybe I should just make the Show in a global before block. Thoughts on that?

@JoelQ
Copy link
Contributor

JoelQ commented Feb 18, 2015

How many specs hit the explore page? I assume we are talking about feature specs?

@r00k
Copy link
Contributor Author

r00k commented Feb 18, 2015

Yep. Just feature specs.

Right now it's one. But I added another in a branch so decided to try this as a preparatory refactoring.

@JoelQ
Copy link
Contributor

JoelQ commented Feb 18, 2015

If it's only one or two places, would just a factory be ok?

@JoelQ
Copy link
Contributor

JoelQ commented Feb 18, 2015

Alternatively, if there are many things that need to be setup just to visit the explore page, how about creating a helper method named setup_explore or something similar?

@JoelQ
Copy link
Contributor

JoelQ commented Feb 18, 2015

It just seems strange to add a Null object that will never be used in production just to remove a line or two from some specs.

@r00k
Copy link
Contributor Author

r00k commented Feb 18, 2015

There's actually already a factory. If a spec needs to hit that page, it uses the factory to create the show.

It just feels a little off to me to need to remember to do that when any spec happens to hit that page.

I take it you're not sold on the null object being worth it? :)

@r00k
Copy link
Contributor Author

r00k commented Feb 18, 2015

It just seems strange to add a Null object that will never be used in production just to remove a line or two from some specs.

Yeah, I agree. I think I'll close this.

@r00k
Copy link
Contributor Author

r00k commented Feb 18, 2015

Thanks for the back-and-forth!

@r00k r00k closed this Feb 18, 2015
@r00k r00k deleted the bo-oi-decouple-weekly-iteration branch February 18, 2015 20:05
@JoelQ
Copy link
Contributor

JoelQ commented Feb 18, 2015

Glad to help!

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.

3 participants