I've been playing around with improvements to how we do tests with plugins.
Currently we use the "standard" Rails Engine way, which is to check out an
app into <plugin>/test and use that as a source of models, controllers etc,
that the plugin modifies and tests.
We go one step further and check out the whole of Foreman into
<plugin>/test/foreman_app (see [1] for an example Rake task), and then run
a database migration as well. This works, but has some drawbacks:
Fragile - I regularly get gem conflicts or database problems during the
setup of the Foreman dummy
Single version - as the code is today, you can only ever test the plugin
against "develop"
Difficult to get started with - because of the above items, there's a
barrier to getting started writing tests for new plugin authors.
It occurred to me that the reason most Rails Engines are written this way
is because they are generic - they aren't meant to be tested against a
specific parent application. However, our plugins are only ever intended to
be for Foreman. So I decided to try and get a plugin to extended Foreman's
own tests.
The results are at [2]. We define a new rake task file, and load it in the
engine.rb so that it shows up in rake -T in Foreman itself:
[greg:~/github/foreman]$ rake -T
…
rake test:default_hostgroup # Run tests for
default_hostgroup
…
This can then be run either standalone or part of rake test (thanks to
the .enhance call). The advantages are:
Clean code - no need for Gemfile, Rakefile, or test_helper in the plugin,
just add files to test/
Extends the version of Foreman that the plugin is loaded in
Automatically included in a full rake test run
Easy to add plugin testing to Jenkins, since it's just "use standard test
job, add plugin, run tests"
There are currently two issues I see, which should be fixable:
Currently no way to separate out controller/unit/integration tests within
the plugin namespace
This just needs some more work in the rake file[3] I think
Setting initializations don't seem to be working correctly - probably
just something stupid on my end
though
I think this is a much nicer way of writing plugin tests - opinions? If we
like it I'll send a PR to the plugin template.
Greg
This is much nicer and great improvement indeed. Do you think I can use
that for running db:migrate and i18n rake tasks as well?
LZ
···
On Tue, Oct 08, 2013 at 06:19:24PM +0100, Greg Sutcliffe wrote:
> Hey all,
>
> I've been playing around with improvements to how we do tests with plugins.
> Currently we use the "standard" Rails Engine way, which is to check out an
> app into /test and use that as a source of models, controllers etc,
> that the plugin modifies and tests.
>
> We go one step further and check out the whole of Foreman into
> /test/foreman_app (see [1] for an example Rake task), and then run
> a database migration as well. This works, but has some drawbacks:
>
> * Fragile - I regularly get gem conflicts or database problems during the
> setup of the Foreman dummy
> * Single version - as the code is today, you can only ever test the plugin
> against "develop"
> * Difficult to get started with - because of the above items, there's a
> barrier to getting started writing tests for new plugin authors.
>
> It occurred to me that the reason most Rails Engines are written this way
> is because they are generic - they aren't meant to be tested against a
> specific parent application. However, our plugins are only ever intended to
> be for Foreman. So I decided to try and get a plugin to extended Foreman's
> own tests.
>
> The results are at [2]. We define a new rake task file, and load it in the
> engine.rb so that it shows up in `rake -T` in Foreman itself:
>
> [greg:~/github/foreman]$ rake -T
> ...
> rake test:default_hostgroup # Run tests for
> default_hostgroup
> ...
>
> This can then be run either standalone or part of `rake test` (thanks to
> the .enhance call). The advantages are:
>
> * Clean code - no need for Gemfile, Rakefile, or test_helper in the plugin,
> just add files to test/
> * Extends the version of Foreman that the plugin is loaded in
> * Automatically included in a full `rake test` run
> * Easy to add plugin testing to Jenkins, since it's just "use standard test
> job, add plugin, run tests"
>
> There are currently two issues I see, which should be fixable:
>
> * Currently no way to separate out controller/unit/integration tests within
> the plugin namespace
> - This just needs some more work in the rake file[3] I think
> * Setting initializations don't seem to be working correctly - probably
> just something stupid on my end
> though
>
> I think this is a much nicer way of writing plugin tests - opinions? If we
> like it I'll send a PR to the plugin template.
> Greg
>
> [1]
> https://github.com/GregSutcliffe/foreman_default_hostgroup/blob/master/Rakefile#L14
> [2]
> https://github.com/GregSutcliffe/foreman_default_hostgroup/commit/29b10d3c63978bb004c62daa58640ca363aa987f
> [3]
> https://github.com/GregSutcliffe/foreman_default_hostgroup/blob/29b10d3c63978bb004c62daa58640ca363aa987f/lib/default_hostgroup.rake
>
> --
> You received this message because you are subscribed to the Google Groups "foreman-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to foreman-dev+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/groups/opt_out.
I think so. You should be able to put any rake task in the plugin
lib/*.rake files, and it will show up in Foreman's rake -T.
One thing I have noticed is that the filenames for the lib/*.rake files
need to be unique, so I think we should have a convention of
lib/<plugin-name>.rake to ensure global uniqueness.
···
On 9 October 2013 08:26, Lukas Zapletal wrote:
This is much nicer and great improvement indeed. Do you think I can use
that for running db:migrate and i18n rake tasks as well?
I agree – this is a much less messier way to execute plugin tests. Nice!
-d
···
On Wed, Oct 9, 2013 at 10:31 AM, Greg Sutcliffe wrote:
On 9 October 2013 08:26, Lukas Zapletal lzap@redhat.com wrote:
This is much nicer and great improvement indeed. Do you think I can use
that for running db:migrate and i18n rake tasks as well?
I think so. You should be able to put any rake task in the plugin
lib/*.rake files, and it will show up in Foreman’s rake -T.
One thing I have noticed is that the filenames for the lib/*.rake files
need to be unique, so I think we should have a convention of
lib/.rake to ensure global uniqueness.
I've spent a bit more time on this, as we identified a fairly major
blocker. ActiveRecord fixtures have no way to specify multiple directories
to load fixtures from, making it impossible for plugins to add their own
fixtures to the list of objects that can be used.
Fortunately, FactoryGirl does permit this. I've tested that using
FactoryGirl (a) allows plugins to add more Factories to use in tests, and
(b) does not break the existing tests (which still use Fixtures). You can
see the changes I made to get it to work at [1] (for core) and [2] (for the
plugin). The rest remains as my original mail (rake test for all tests,
including the plugin, or rake test:<pluginname> for just the plugin tests),
both work as before. The changes are somewhat contrived (one factory in
core, one in the plugin), as the plugin I'm working on has no new models to
create factories for, but it proves the idea works.
Ohad/Joseph/Dominic: We've spoken about switching to FactoryGirl, but we
didn't have a compelling reason to switch. I think that this now a good
reason - as it breaks nothing, I suggest I send a PR for the changes to
core shown here, and we can start coding new tests using FactoryGirl.
Existing ones can be converted as we have time and energy (since they're
not broken by this change, we can take it easy). How does that sound?
···
On Oct 15, 2013 1:20 PM, "Greg Sutcliffe" wrote:
I’ve spent a bit more time on this, as we identified a fairly major
blocker. ActiveRecord fixtures have no way to specify multiple directories
to load fixtures from, making it impossible for plugins to add their own
fixtures to the list of objects that can be used.
Fortunately, FactoryGirl does permit this. I’ve tested that using
FactoryGirl (a) allows plugins to add more Factories to use in tests, and
(b) does not break the existing tests (which still use Fixtures). You can
see the changes I made to get it to work at [1] (for core) and [2] (for the
plugin). The rest remains as my original mail (rake test for all tests,
including the plugin, or rake test: for just the plugin tests),
both work as before. The changes are somewhat contrived (one factory in
core, one in the plugin), as the plugin I’m working on has no new models to
create factories for, but it proves the idea works.
Ohad/Joseph/Dominic: We’ve spoken about switching to FactoryGirl, but we
didn’t have a compelling reason to switch. I think that this now a good
reason - as it breaks nothing, I suggest I send a PR for the changes to
core shown here, and we can start coding new tests using FactoryGirl.
Existing ones can be converted as we have time and energy (since they’re
not broken by this change, we can take it easy). How does that sound?
I've not explicitly tested it, but as far as I can tell from reading the
Rails4 codebase - no. The fixture handling code looks very similar to
Rails3, especially around the fixture_path handling. It still appears to be
expecting a single path, not an array.
>
>> Do you know if Rails 4 addresses this?
>>
> I've not explicitly tested it, but as far as I can tell from reading the
> Rails4 codebase - no. The fixture handling code looks very similar to
> Rails3, especially around the fixture_path handling. It still appears to be
> expecting a single path, not an array.
>
can we monkey patch it? long term, best to send a PR to rails.
Ohad
···
On Tue, Oct 15, 2013 at 9:53 PM, Greg Sutcliffe wrote:
> On 15 October 2013 19:30, Eric D Helms wrote:
>
> --
> You received this message because you are subscribed to the Google Groups
> "foreman-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to foreman-dev+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/groups/opt_out.
>
I’ve not explicitly tested it, but as far as I can tell from reading the
Rails4 codebase - no. The fixture handling code looks very similar to
Rails3, especially around the fixture_path handling. It still appears to be
expecting a single path, not an array.
can we monkey patch it? long term, best to send a PR to rails.
While possible (not sure about monkey patching though), I think it would
require a reasonable amount of effort. What's the objection to FactoryGirl?
-d
···
On Tue, Oct 15, 2013 at 8:45 PM, Greg Sutcliffe wrote:
I’ve not explicitly tested it, but as far as I can tell from reading the
Rails4 codebase - no. The fixture handling code looks very similar to
Rails3, especially around the fixture_path handling. It still appears to be
expecting a single path, not an array.
can we monkey patch it? long term, best to send a PR to rails.
Anything’s possible, but I was hoping to be done with this If you feel
that would be better, I can try and work something out tomorrow.
> While possible (not sure about monkey patching though), I think it would
> require a reasonable amount of effort. What's the objection to FactoryGirl?
>
No real objections to FG, my thinking was:
rails can be improved, lets do it.
if its only a small path change - e.g. to a Dir['test/fixtures/*' ]
somewhere, we might be able to get away with a very simple MP and send back
a PR (as it seems Greg already found that code).
Ohad
···
On Tue, Oct 15, 2013 at 10:50 PM, Dmitri Dolguikh wrote:
I’ve not explicitly tested it, but as far as I can tell from reading
the Rails4 codebase - no. The fixture handling code looks very similar to
Rails3, especially around the fixture_path handling. It still appears to be
expecting a single path, not an array.
can we monkey patch it? long term, best to send a PR to rails.
Anything’s possible, but I was hoping to be done with this If you feel
that would be better, I can try and work something out tomorrow.
It's not a small change, there a lot of methods in fixtures.rb that consume
the fixtures_path as string interpolation, like "foo/#{fixtures_path}/bar"
all of these would need rewriting to handle an array of paths properly. I
was trying to figure out if it could be easily overriden before I tested
FactoryGirl.
I can take a closer look if we think it's worth the time & effort though.
···
On 16 October 2013 06:43, Ohad Levy wrote:
if its only a small path change - e.g. to a Dir[‘test/fixtures/*’ ]
somewhere, we might be able to get away with a very simple MP and send back
a PR (as it seems Greg already found that code).
If you feel