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

[Generators] ActiveJob #1155

Merged
merged 1 commit into from Aug 27, 2014

Conversation

Projects
None yet
5 participants
@seuros
Member

seuros commented Aug 22, 2014

WIP !

@seuros

This comment has been minimized.

Member

seuros commented Aug 22, 2014

@myronmarston Since this generator is only present on 4.2, how can i test it ?

@myronmarston

This comment has been minimized.

Member

myronmarston commented Aug 22, 2014

@cupakromer maintains rspec-rails and is a better person to direct your question towards.

@cupakromer

This comment has been minimized.

Member

cupakromer commented Aug 22, 2014

@seuros are you asking about how to add specs for rspec-rails for your code? Or is it a more general question about how you should test ActiveJob in your projects?

@seuros

This comment has been minimized.

Member

seuros commented Aug 22, 2014

@cupakromer , the current PR is working. But as i see that all other generators have specs.
The issue here is that AJ is not available in other version of rails. So if i add a test, it will fall in all but rails 4.2

@seuros

This comment has been minimized.

Member

seuros commented Aug 22, 2014

Maybe i should just skip the tests if rails version < 4.2 ?

@cupakromer

This comment has been minimized.

Member

cupakromer commented Aug 22, 2014

Hmm, yes, I do loath doing version checks, but sometimes it's unavoidable. Let me think on that a little and I'll reply later tonight / tomorrow with my thoughts. I do have some thoughts I want to test out locally.

Also, please be aware that right now Rails 4.2. builds are going to fail until we merge #1149. I expect this to be merged very soon (tonight or tomorrow). I'm in the process of seeing if we can get a new patch release for ammeter in that time frame. If not, I'll merge it as is pointing at my github branch.

@seuros

This comment has been minimized.

Member

seuros commented Aug 22, 2014

Fine. You have to use rails master for tests. I just added few hours ago the hook.

@seuros seuros force-pushed the seuros:master branch 2 times, most recently from 84725ee to 10f3204 Aug 25, 2014

@seuros

This comment has been minimized.

Member

seuros commented Aug 25, 2014

@cupakromer I think we are good now.

def has_activejob?
Rails::VERSION::STRING >= '4.2'
end

This comment has been minimized.

@JonRowe

JonRowe Aug 25, 2014

Member

I'd prefer this was put into a module and referenced as such.

This comment has been minimized.

@seuros

seuros Aug 25, 2014

Member

I changed it few minutes ago to defined?(ActiveJob)

This comment has been minimized.

@JonRowe

JonRowe Aug 25, 2014

Member

Not pushed though ;)

This comment has been minimized.

@seuros

seuros Aug 25, 2014

Member

Done.

@JonRowe

This comment has been minimized.

Member

JonRowe commented Aug 25, 2014

Looks good to me, but it'd be nice if it was squashed with a descriptive commit message.

@seuros seuros force-pushed the seuros:master branch 2 times, most recently from bc3f8b7 to 9c77e43 Aug 25, 2014

@seuros

This comment has been minimized.

Member

seuros commented Aug 25, 2014

Done!

def has_activejob?
defined?(ActiveJob)
end

This comment has been minimized.

@JonRowe

JonRowe Aug 25, 2014

Member

It'd still be nice to have this defined in a module and referenced that way... e.g:

module RailsFeatures
  def has_activeob?
  end
  module_function :has_activejob?
end

RSpec.describe ... :skip => !RailsFeatures.has_activejob?

@seuros seuros force-pushed the seuros:master branch 2 times, most recently from 7695ef9 to 3fcf2fc Aug 25, 2014

def has_activejob?
defined?(::ActiveJob)
end
end

This comment has been minimized.

@cupakromer

cupakromer Aug 25, 2014

Member

I'm not a fan of this. In general I'd prefer to see this use module_function instead. Though I'm not sure this is the right way to go just yet.

This comment has been minimized.

@seuros

seuros Aug 25, 2014

Member

I will have used function_module, but ruby 1.8.7 don't know that.

This comment has been minimized.

@JonRowe

JonRowe Aug 25, 2014

Member

Yep, please don't extend self, @cupakromer there's a need to exclude the spec from the build, I'd support either an inline version check or a features module like we do in RSpec itself.

This comment has been minimized.

@JonRowe

JonRowe Aug 25, 2014

Member

module_function works fine on 1.8.7, see here

This comment has been minimized.

@cupakromer

cupakromer Aug 25, 2014

Member

I'm aware of the need, just thinking about alternative ways to do that.

This comment has been minimized.

@seuros

seuros Aug 25, 2014

Member

@JonRowe my bad. I had a typo.

# Generators are not automatically loaded by Rails
require 'generators/rspec/job/job_generator'
RSpec.describe Rspec::Generators::JobGenerator, :type => :generator, :skip => !RailsFeatures.has_activejob? do

This comment has been minimized.

@cupakromer

cupakromer Aug 25, 2014

Member

I know I said that in general I prefer not to have version check flags. Though sometimes I feel that is an easier to understand and more general solution. I do think your way here is a good idea. Though I'm wondering if a more generic type of metadata would be more beneficial.

Maybe something like;

:min_rails_version => 4.2

Then we just have a filter which excludes such specs if the running Rails version doesn't fit. Thoughts?

This comment has been minimized.

@seuros

seuros Aug 25, 2014

Member

I like the syntax, i can work on this feature soon.

This comment has been minimized.

@JonRowe

JonRowe Aug 25, 2014

Member

@cupakromer I think that would add to much complexity for skipping just one spec :P

This comment has been minimized.

@cupakromer

cupakromer Aug 25, 2014

Member

It's not just one spec. We have several places where we already do a version check. I'd prefer to think of a generic way, if possible to do cleanly, to generalize this. Also, we know other changes are coming done the line. It's just a question of when.

This comment has been minimized.

@JonRowe

JonRowe Aug 25, 2014

Member

I'm aware of that but it still seems like overkill to me, also this would guarantee the feature was available when it exists, regardless of version, which seems more future proof to me.

This comment has been minimized.

@cupakromer

cupakromer Aug 27, 2014

Member

Actually, we don't need this feature check at all here. Not unless we don't load the generator unless we check the feature too. All the generator will do is create the spec file. It doesn't create the actual job object. So, there's no real dependency with Rails here. If we want this only to be available if ActiveJob is, then the generator itself should not be loaded unless ActiveJob is loaded.

This comment has been minimized.

@seuros

seuros Aug 27, 2014

Member

I think it better to keep the current way. Right now we see that the test is skipped.
But if we don't load the generator at all, and ActiveJob is not defined for some other reasons, the error will be swallowed .

@seuros seuros force-pushed the seuros:master branch from 3fcf2fc to 1f07f1a Aug 25, 2014

@cupakromer

This comment has been minimized.

Member

cupakromer commented Aug 25, 2014

We should add this into generator docs in features/Generators.md. Also, it would be good to call this during the smoke tasks. Adding a line (with proper toggle) to templates/generate_stuff.rb (though this will change with #1150): generate('job does_something').

I took a very quick look at the code and I didn't see any obvious additions to ActiveSupport::TestCase to provide custom setup for job tests. So I think we are probably good with this simple generator.

@seuros

This comment has been minimized.

Member

seuros commented Aug 25, 2014

Done! There is probably also the cucumber specs, but i won't touch that. I'm a carnivore :trollface: .

I'm adding TestCase for AJ, i will update this generator if required.

@cupakromer

This comment has been minimized.

Member

cupakromer commented Aug 27, 2014

I thought about this a bit more. I'm now more sold on the feature toggles. I agree it is helpful for future proofing.

My only remaining concern is we won't be as aware when things have changed and require action on our part. This isn't really an issue if a feature is removed, things work as desired in this case. However, if something needs updating, we won't be made aware since the specs simply won't run.

I'll leave a few more comments about specific code changes.

@seuros

This comment has been minimized.

Member

seuros commented Aug 27, 2014

We will notice that, since RSpec show skipped specs.

def has_activejob?
defined?(::ActiveJob)
end
end

This comment has been minimized.

@cupakromer

cupakromer Aug 27, 2014

Member

Can we move this into the lib directory and place it under the RSpec::Rails module to avoid potential naming conflicts? Perhaps:

module RSpec
  module Rails
    module FeatureChecks
      # ...
    end
  end
end

There are some parts of the codebase I'd like to start leveraging this on.

This comment has been minimized.

@seuros

seuros Aug 27, 2014

Member

what about

module RSpec
  module Rails
    module Components

These are not features.

This comment has been minimized.

@cupakromer

cupakromer Aug 27, 2014

Member

They are features of rails. I plan on expanding this to checks other than just for classes. Such as checks for specific methods on a class or module. Usually these are called "feature flags" or something similar.

@seuros

This comment has been minimized.

Member

seuros commented Aug 27, 2014

unrelated to this PR, why there is no routing generator ?

@seuros seuros force-pushed the seuros:master branch from 1f07f1a to a1a6b39 Aug 27, 2014

@seuros seuros force-pushed the seuros:master branch from a1a6b39 to 5646d1f Aug 27, 2014

@cupakromer

This comment has been minimized.

Member

cupakromer commented Aug 27, 2014

We will notice that, since RSpec show skipped specs.

That's not as noticeable as a failing spec. No one really looks at all of the output from the Travis CI builds, so unless the build is failing red, it won't get noticed. Regarding local builds, that would really depend on which version(s) the dev was using at the time. It's very likely things ran locally and weren't skipped. It's also possible that the skipped spec wasn't noticed since it didn't fail the spec run.

@cupakromer

This comment has been minimized.

Member

cupakromer commented Aug 27, 2014

unrelated to this PR, why there is no routing generator ?

AFAIK, there hasn't been a need. Rails doesn't provide a routes generator. The majority of the rspec-rails generators are designed to piggy-back off of the out of the box rails generators. They helpfully provide the test parity for the created rails object.

Along these lines, the existing rspec-rails route generator is part of the scaffold generator. This makes sense as it can provide all the helpful route specs by default. Otherwise, routing tends to be fairly non-standard and I'm not sure there's much benefit in creating a generator that doesn't hook into something else.

@cupakromer

This comment has been minimized.

Member

cupakromer commented Aug 27, 2014

I don't think my concern about the generator always being available is a merge blocker. If this goes green I'll merge. Thanks ❤️ for this @seuros!

@seuros

This comment has been minimized.

Member

seuros commented Aug 27, 2014

Rails just don't drop/add a feature without warning. I can maintain this part if you want.

As for the route generator, it should create the file every time a controller is generated.

@cupakromer

This comment has been minimized.

Member

cupakromer commented Aug 27, 2014

Rails just don't drop/add a feature without warning

To a degree. It does sometimes change APIs without notice. I'm happy to learn better ways to keep up with potential changes. Generally, an angry 😡 test suite has been the most helpful for me personally.

it should create the file every time a controller is generated

Not all controllers have routes. However, it does seem that if you use the Rails generator to create a controller with some actions, it will automatically add basic routes for it. 😺 Since that is the case, I'm happy to see a PR add this in. It would just need to do two things:

  1. Automatically add passing specs for the generated routes
  2. If no actions / routes were specified, include our default filler message
@seuros

This comment has been minimized.

Member

seuros commented Aug 27, 2014

I'm happy to learn better ways to keep up with potential changes.

Changelog. Any behavior change/bug fix won't be merged without updating the changelog.

cupakromer added a commit that referenced this pull request Aug 27, 2014

Merge pull request #1155 from seuros/master
[Generators] ActiveJob

@cupakromer cupakromer merged commit 3921b9d into rspec:master Aug 27, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@cupakromer

This comment has been minimized.

Member

cupakromer commented Aug 27, 2014

Thanks @seuros! ❤️

@cupakromer

This comment has been minimized.

Member

cupakromer commented Aug 29, 2014

@seuros I'm adding acceptance specs for this in #1150. However, in generating the sample app and files I've noticed a few things:

  • Calling rails:template is not loading ActiveJob by default, I need to manually require it

  • The rails generator isn't calling the RSpec generator for some reason:

    $ bin/rails -v; bin/rails g job something
    Rails 4.2.0.beta1
          create  app/jobs/something_job.rb
@seuros

This comment has been minimized.

Member

seuros commented Aug 29, 2014

Beta1 won't work, i added the hook after the release. Try with master.

@cupakromer

This comment has been minimized.

Member

cupakromer commented Aug 29, 2014

@seuros

This comment has been minimized.

Member

seuros commented Aug 29, 2014

Np @cupakromer :). I can add some matchers in the incoming days.

@cupakromer

This comment has been minimized.

Member

cupakromer commented Aug 29, 2014

Sounds good. @seuros if you have any questions about that, or how to try to generalize things for feature specs, etc. let me know.

@Onumis

This comment has been minimized.

Onumis commented Mar 5, 2015

Are there any plans on supporting helpers like http://api.rubyonrails.org/classes/ActiveJob/TestHelper.html ?

I'd expect to be able to do this for example:
expect(enqueued_jobs).to be(0)

@cupakromer

This comment has been minimized.

Member

cupakromer commented Mar 5, 2015

@Onumis there is now: #1334 ❤️

@Onumis

This comment has been minimized.

Onumis commented Mar 5, 2015

@cupakromer you're the men! 🏆

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