Creating a plugin-for-tests (was Plugin test failures)

Gwmngilfen · November 3, 2016, 1:04pm

This came up on 3rd Oct, but I think the second half of Stephen's email got
lost in the discussion of the immediate issues. To summarize:

> From: Stephen Benjamin <stephen@redhat.com>
> Date: 3 October 2016 at 21:28
> Subject: [foreman-dev] Plugin test failures
> To: Foreman <foreman-dev@googlegroups.com>
>
> Would it be possible to get a standard plugin in Foreman's test matrix
for PR? Perhaps tasks is a good candidate. Not on all databases/rubies,
but something might help gain awareness of the impact of certain changes on
plugins. Katello is there, but it doesn't run all (any?) of Foreman's tests.

I think this is a good plan. I'd be happy to see tasks or maybe salt added
to the test matrix in the short term. I also think it might make sense to
have a dedicated "fake" plugin that touches a whole ton of stuff just for
the sake of knowing if we've broken it. We could look to that for the
longer term.

> Along side that, we should be tracking what plugin maintainers need to do
to update from one release to the next.

This is good too - I'll be honest and say that my plugins don't seem to
break as often as yours (just luck I guess) but it is useful when people
come tell me what needs updating.

I might be wrong, but I don't think we merge breaking changes after
freeze-date, so it should be possible to track this and then tell authors
about the changes when the first RCs go out - but first we need the "fake"
plugin that touches tons of stuff, so we know what the diffs look like (we
could even just send a list of commits to the fake plugin, showing how to
change the methods).

I can see the main concerns being:

Test resources - do we have enough to run this on every PR? Do we want
to, or should it be less frequent?
Should it block merges, or be advisory only? If yes, do PR authors need
to prepare patches for the plugin when they break a plugin?

I'm happy to write this up as an RFC if others think its worth having a
more formal discussion, or if one of the people with access wants to give
running foreman_tasks/salt on PRs a trial, that's cool too.

Greg

···

> ---------- Forwarded message ----------

Dominic_Cleal2 · November 3, 2016, 1:19pm

>> From: Stephen Benjamin <stephen@redhat.com <mailto:stephen@redhat.com>>
>> Date: 3 October 2016 at 21:28
>> Subject: [foreman-dev] Plugin test failures
>> To: Foreman <foreman-dev@googlegroups.com
> <mailto:foreman-dev@googlegroups.com>>
>>
>> Would it be possible to get a standard plugin in Foreman's test matrix
> for PR? Perhaps tasks is a good candidate. Not on all databases/rubies,
> but something might help gain awareness of the impact of certain changes
> on plugins. Katello is there, but it doesn't run all (any?) of Foreman's
> tests.
>
> I think this is a good plan. I'd be happy to see tasks or maybe salt
> added to the test matrix in the short term. I also think it might make
> sense to have a dedicated "fake" plugin that touches a whole ton of
> stuff just for the sake of knowing if we've broken it. We could look to
> that for the longer term.

Foreman already has a test suite for its plugin interfaces which is run
on every pull request. If those tests are insufficient, expand them.

> I might be wrong, but I don't think we merge breaking changes after
> freeze-date, so it should be possible to track this and then tell
> authors about the changes when the first RCs go out

Deprecations affecting plugins are listed in release notes today, though
some plugin authors don't seem to bother changing things until the
interfaces are later removed.

> 1) Test resources - do we have enough to run this on every PR? Do we
> want to, or should it be less frequent?

There's no need. The Foreman plugin interface is already tested on every
PR, and plugin tests themselves are run at least weekly - authors should
be aware of issues quickly.

You should file a bug against Foreman immediately if you see that a
plugin interface has been broken, so we can ensure the change isn't
released and a fix can be written.

···

On 03/11/16 13:04, Greg Sutcliffe wrote: >> ---------- Forwarded message ----------

–
Dominic Cleal
dominic@cleal.org

Gwmngilfen · November 3, 2016, 1:50pm

Thanks for the info Dominic. Some further questions:

> Foreman already has a test suite for its plugin interfaces which is run
> on every pull request. If those tests are insufficient, expand them.
>

Is that in
https://github.com/theforeman/foreman/blob/develop/test/unit/plugin_test.rb?

I see we have a section in the wiki on Jenkins for the plugins themselves,
but
nothing about these tests - it's probably worth adding a section about (a)
how
to be aware/notified of potential breakage (release notes, and failures in
these tests),
and (b) if, as an author, you extend something not covered by these tests,
then
consider sending a PR to core to make sure it's covered. I'm happy to do
that, if
that makes sense.

> Deprecations affecting plugins are listed in release notes today, though
> some plugin authors don't seem to bother changing things until the
> interfaces are later removed.
>

We can't force authors to update, sure. The release notes go out with the
RCs right?

> 1) Test resources - do we have enough to run this on every PR? Do we
> > want to, or should it be less frequent?
>
> There's no need. The Foreman plugin interface is already tested on every
> PR, and plugin tests themselves are run at least weekly - authors should
> be aware of issues quickly.
>

Yeah, that makes sense. I wasn't aware of those tests - so long as something
is testing the interfaces, that's fine.

> You should file a bug against Foreman immediately if you see that a
> plugin interface has been broken, so we can ensure the change isn't
> released and a fix can be written.
>

Fair enough - in this case it's an interface that wasn't tested and a user
has found
it subsequently (#17203 in set_hostgroup_defaults), so I think the
not-releasing-it
option is gone. I'll open an issue to write some tests for that interface,
though.

More generally, does it make sense for someone to do an audit of the most
common
places that plugins hook in, and make sure we have tests for them?

Cheers
Greg

···

On 3 November 2016 at 13:19, Dominic Cleal wrote:

ohadlevy · November 3, 2016, 2:16pm

I don't think its a lack of tests, but rather someone updates a method and
its tests for it without realizing its used by a plugin.

I wonder if there is a way to do code coverage of plugins just like you can
do for tests?

···

On Thu, Nov 3, 2016 at 3:50 PM, Greg Sutcliffe wrote:

More generally, does it make sense for someone to do an audit of the most
common
places that plugins hook in, and make sure we have tests for them?

dLobatog · November 4, 2016, 11:48am

>
>
> > You should file a bug against Foreman immediately if you see that a
> > plugin interface has been broken, so we can ensure the change isn't
> > released and a fix can be written.
> >
>
> Fair enough - in this case it's an interface that wasn't tested and a user
> has found
> it subsequently (#17203 in set_hostgroup_defaults), so I think the
> not-releasing-it
> option is gone. I'll open an issue to write some tests for that interface,
> though.
>
> More generally, does it make sense for someone to do an audit of the most
> common
> places that plugins hook in, and make sure we have tests for them?

The 'problem' is that the places plugins hook in are usually parts of
the app, e.g 'Host.foo', 'computeresource.bar'. The defined endpoints
for the plugin API are usually fine, but other methods that are equally
as important to the plugin could change behavior unexpectedly.

Personally I think from core the important part is being aware of these
changes and communicating them promptly - helping to fix them when
possible too.

···

On 11/03, Greg Sutcliffe wrote:

Cheers
Greg

–
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/d/optout.

–
Daniel Lobato Garcia

@dLobatog
blog.daniellobato.me
daniellobato.me

GPG: http://keys.gnupg.net/pks/lookup?op=get&search=0x7A92D6DD38D6DE30
Keybase: https://keybase.io/elobato

Shimon_Shtein · November 6, 2016, 8:34am

+1 for Daniel.

IMHO most of the breakages that I saw were related to changes in the code
that were not "officially declared" as API. Good examples are
alias_method_chain for various methods.

···

On Friday, November 4, 2016 at 1:48:13 PM UTC+2, Daniel Lobato wrote: > > On 11/03, Greg Sutcliffe wrote: > > > > > > > You should file a bug against Foreman immediately if you see that a > > > plugin interface has been broken, so we can ensure the change isn't > > > released and a fix can be written. > > > > > > > Fair enough - in this case it's an interface that wasn't tested and a > user > > has found > > it subsequently (#17203 in set_hostgroup_defaults), so I think the > > not-releasing-it > > option is gone. I'll open an issue to write some tests for that > interface, > > though. > > > > More generally, does it make sense for someone to do an audit of the > most > > common > > places that plugins hook in, and make sure we have tests for them? > > The 'problem' is that the places plugins hook in are usually parts of > the app, e.g 'Host.foo', 'computeresource.bar'. The defined endpoints > for the plugin API are usually fine, but other methods that are equally > as important to the plugin could change behavior unexpectedly. > > Personally I think from core the important part is being aware of these > changes and communicating them promptly - helping to fix them when > possible too. > > > > > Cheers > > Greg > > > > -- > > 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...@googlegroups.com . > > For more options, visit https://groups.google.com/d/optout. > > -- > Daniel Lobato Garcia > > @dLobatog > blog.daniellobato.me > daniellobato.me > > GPG: http://keys.gnupg.net/pks/lookup?op=get&search=0x7A92D6DD38D6DE30 > Keybase: https://keybase.io/elobato >

stbenjam · November 7, 2016, 3:39pm

> From: sshtein@redhat.com
> To: "foreman-dev" <foreman-dev@googlegroups.com>
> Sent: Sunday, November 6, 2016 3:34:40 AM
> Subject: Re: Re: Creating a plugin-for-tests (was [foreman-dev] Plugin test failures)
>
> +1 for Daniel.
>
> IMHO most of the breakages that I saw were related to changes in the code
> that were not "officially declared" as API. Good examples are
> alias_method_chain for various methods.

The wiki says that public methods will be deprecated and given a chance
to adjust our code in a reasonable time where possible:

&quot;Foreman will always strive to make no incompatible changes in a minor release, but be prepared
to make updates on major releases. Where possible, deprecation warnings will be added for old
public methods before their removal. Warnings will be issued for two major releases and then the
old method removed in the third release, giving plenty of time to update plugins.&quot;

I read that as being quasi-official to be able to do alias_method_chain on
public methods, but very often - nearly always - they are not deprecated and
the signature just changes or maybe the method is even removed. It constantly breaks
my plugin.

The last time I had test green, they were green for ONE DAY. There's no consideration
at all for changes to Foreman's public methods.

And the plugin API (which itself is a mess given it grows organically and ad hoc instead
of some team spending time to get it right) is pretty limited if you want
to add on to Foreman objects.

Being a plugin maintainer really sucks.

Stephen

···

----- Original Message -----

On Friday, November 4, 2016 at 1:48:13 PM UTC+2, Daniel Lobato wrote:

On 11/03, Greg Sutcliffe wrote:

You should file a bug against Foreman immediately if you see that a
plugin interface has been broken, so we can ensure the change isn’t
released and a fix can be written.

Fair enough - in this case it’s an interface that wasn’t tested and a
user
has found
it subsequently (#17203 in set_hostgroup_defaults), so I think the
not-releasing-it
option is gone. I’ll open an issue to write some tests for that
interface,
though.

More generally, does it make sense for someone to do an audit of the
most
common
places that plugins hook in, and make sure we have tests for them?

The ‘problem’ is that the places plugins hook in are usually parts of
the app, e.g ‘Host.foo’, ‘computeresource.bar’. The defined endpoints
for the plugin API are usually fine, but other methods that are equally
as important to the plugin could change behavior unexpectedly.

Personally I think from core the important part is being aware of these
changes and communicating them promptly - helping to fix them when
possible too.

Cheers
Greg

–
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...@googlegroups.com <javascript:>.
For more options, visit https://groups.google.com/d/optout.

–
Daniel Lobato Garcia

@dLobatog
blog.daniellobato.me
daniellobato.me

GPG: http://keys.gnupg.net/pks/lookup?op=get&search=0x7A92D6DD38D6DE30
Keybase: https://keybase.io/elobato

–
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/d/optout.

Dominic_Cleal2 · November 8, 2016, 10:42am

> The wiki says that public methods will be deprecated and given a chance
> to adjust our code in a reasonable time where possible:
>
> "Foreman will always strive to make no incompatible changes in a minor release, but be prepared
> to make updates on major releases. Where possible, deprecation warnings will be added for old
> public methods before their removal. Warnings will be issued for two major releases and then the
> old method removed in the third release, giving plenty of time to update plugins."
>
> I read that as being quasi-official to be able to do alias_method_chain on
> public methods

No, this means public methods should still be able to be called, not
redefined. There's no mention of support for redefining methods via
alias_method_chain (or others) in the plugin documentation.

There's little Foreman can do if a method is being redefined and is
later deprecated in favour of something else, since Foreman would no
longer call the deprecated method. The deprecation is only useful when
it's called, not modified.

> but very often - nearly always - they are not deprecated and
> the signature just changes or maybe the method is even removed. It constantly breaks
> my plugin.

Be more defensive against signature changes by redefining the method
with a splat operator, e.g. def foo(*args) and passing *args to the
original method. If arguments with defaults are added, no change is
required to the redefined method.

> And the plugin API (which itself is a mess given it grows organically and ad hoc instead
> of some team spending time to get it right) is pretty limited if you want
> to add on to Foreman objects.

Please expand it then, or at least ensure a ticket is filed in the
Plugins category in Foreman's issue tracker for the functionality you
require.

It's good that it's ad-hoc because those are the extensions points that
plugin authors have needed, and they can be specifically tested.

···

On 07/11/16 15:39, Stephen Benjamin wrote:

–
Dominic Cleal
dominic@cleal.org

stbenjam · November 8, 2016, 2:56pm

> From: "Dominic Cleal" <dominic@cleal.org>
> To: foreman-dev@googlegroups.com
> Sent: Tuesday, November 8, 2016 5:42:04 AM
> Subject: Re: Creating a plugin-for-tests (was [foreman-dev] Plugin test failures)
>
> > The wiki says that public methods will be deprecated and given a chance
> > to adjust our code in a reasonable time where possible:
> >
> > "Foreman will always strive to make no incompatible changes in a minor
> > release, but be prepared
> > to make updates on major releases. Where possible, deprecation warnings
> > will be added for old
> > public methods before their removal. Warnings will be issued for two
> > major releases and then the
> > old method removed in the third release, giving plenty of time to
> > update plugins."
> >
> > I read that as being quasi-official to be able to do alias_method_chain on
> > public methods
>
> No, this means public methods should still be able to be called, not
> redefined. There's no mention of support for redefining methods via
> alias_method_chain (or others) in the plugin documentation.
>
> There's little Foreman can do if a method is being redefined and is
> later deprecated in favour of something else, since Foreman would no
> longer call the deprecated method. The deprecation is only useful when
> it's called, not modified.
>
> > but very often - nearly always - they are not deprecated and
> > the signature just changes or maybe the method is even removed. It
> > constantly breaks
> > my plugin.
>
> Be more defensive against signature changes by redefining the method
> with a splat operator, e.g. def foo(*args) and passing *args to the
> original method. If arguments with defaults are added, no change is
> required to the redefined method.
>
> > And the plugin API (which itself is a mess given it grows organically and
> > ad hoc instead
> > of some team spending time to get it right) is pretty limited if you want
> > to add on to Foreman objects.
>
> Please expand it then, or at least ensure a ticket is filed in the
> Plugins category in Foreman's issue tracker for the functionality you
> require.
>
> It's good that it's ad-hoc because those are the extensions points that
> plugin authors have needed, and they can be specifically tested.

I strongly disagree with this. One thing we see now is as they get added
organically, every single one works in a different way and stores the
plugin configuration in a different way, often resulting in bugs. Just
look at this and the attached bugs: Refactor #12747: Provide some kind of central store or consistent way to manage dynamically registered objects from plugins - Foreman

We don't only add API calls whenever a need to use it comes up, likewise
obviously important objects should have plugin extension points.

I suggested adding a representative plugin to the test matrix so users were
aware that particular methods are used by plugins and maybe find a way to
avoid changing it, at least until the plugin extension points are better.
There is definitely a common set of things that people are alias_method_chaining or
hooking onto to add objects to host groups, hosts, title bar actions, etc.

But the answer from this thread is basically, "too bad." So I'm just going
to stop trying to plug the holes in the drowning ship that is foreman_salt.

Stephen

···

----- Original Message ----- > On 07/11/16 15:39, Stephen Benjamin wrote:

–
Dominic Cleal
dominic@cleal.org

–
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/d/optout.