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

Fix bug where routes helpers cannot be used in tests that use an anonymous controller. #881

Closed
wants to merge 1 commit into from
Closed

Conversation

thomasfedb
Copy link
Contributor

Changed orig_routes to @orig_routes as it is referred to in method_missing on line 138. Bug introduced in 86aea05.

I attempted to write a failing test, but it was beyond me how to do so. It does, however resolve the incorrectly failing test in my application.

…n an

anonymous controller has been defined.
@thomasfedb
Copy link
Contributor Author

Note: I have made this change against the commit for 3.0.0.beta1 so the branch can be used as a straight replacement. The change should apply cleanly to master however.

@JonRowe
Copy link
Member

JonRowe commented Dec 17, 2013

If you can find a way to make this change that doesn't cause warnings to be issued (the cause of 86aea05) then I'll merge it, otherwise we need to find a way to fix this that doesnt generate warnings.

@thomasfedb
Copy link
Contributor Author

This doesn't currently cause any warnings for me. (ruby 2.0.0p353)

@JonRowe
Copy link
Member

JonRowe commented Dec 17, 2013

I'll check it out later, but be mindful that we support numerous versions of Rails and Ruby.

@ghost ghost assigned JonRowe Dec 17, 2013
@thomasfedb
Copy link
Contributor Author

@JonRowe Yes, I saw the list on Travis - impressive.

However, personally I'd take fixing a bug over avoiding a warning, especially when the bug breaks tests that previously passed.

@chuyeow
Copy link

chuyeow commented Dec 20, 2013

I can confirm that this fixes a test that broke after I upgraded to 3.0.0.beta1 from 2.99.0.beta1.

FWIW the test that now fails looks something like this:

describe ApplicationController do
  controller do
    def index
      render nothing: true
    end
  end

  describe '#authenticate_user!' do
    it 'redirects to a confirmation controller' do
      get :index
      expect(controller).to redirect_to(new_confirmation_path) # new_confirmation_path is undefined
    end
end

@thomasfedb
Copy link
Contributor Author

Very similar spec to the one I have @chuyeow. Can you confirm that it works against my branch?

@chuyeow
Copy link

chuyeow commented Dec 20, 2013

Yup the test passes after I pull your patch.

@chuyeow
Copy link

chuyeow commented Dec 20, 2013

FWIW again, +1 to this PR. I really want to get rid of that single failing test in my test suite :)

I agree with @thomasfedb that fixing this bug is more important than the warnings.

@JonRowe
Copy link
Member

JonRowe commented Dec 20, 2013

When this has a test that fails, and it fixes the warning too I'll merge it

On Friday, 20 December 2013, Cheah Chu Yeow wrote:

FWIW again, +1 to this PR. I really want to get rid of that single failing
test in my test suite :)

I agree with @thomasfedb https://github.com/thomasfedb that fixing this
bug is more important than the warnings.


Reply to this email directly or view it on GitHubhttps://github.com//pull/881#issuecomment-30994153
.

@JonRowe
Copy link
Member

JonRowe commented Dec 24, 2013

Ok, I've fixed up a lot of warnings on rspec-rails and enabled our warning detection from rspec-support over in #892, can you rebase this off that, and add a spec showing your bug, then I'll merge this if it's green.

@thomasfedb
Copy link
Contributor Author

Please continue at #905.

@thomasfedb thomasfedb closed this Jan 3, 2014
@thomasfedb thomasfedb deleted the controller-fix branch January 5, 2014 22:42
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.

None yet

3 participants