Fixes #6549 - Add :tests_to_skip to plugin registration block

[GregSutcliffe]

This PR allows a plugin to do something like

Foreman::Plugin.register :foreman_discovery do                                 
  tests_to_skip "EncryptableTest" => ["this string IS encryptable and NOT decryptable"],
                "HostTest" => ["should not save without a hostname"]
end

i.e. a map of test types to arrays of test names to skip

This is detected where test (which is aliased against it later in the test_helper) is defined - we jump in and monkeypatch MiniTest to check if this is a prohibited test class+name. If it is, we just define the test as a skip.
This alters the array of tests to run by using a custom test runner. We step into the _run_suite method and add an extra round of filtering against Foreman::Plugin.tests_to_skip

This is slight hacky - the downside is:

1. monkey_patching MiniTest replacing _run_suite and not calling super
2. it only works for the newer test "foo" ... end style tests, def test_foo ... end can't be caught this way fixed.

Feedback welcome - as I'm out of ideas as to how to do it better :)

[ehelms]

One idea could be to use a custom test runner similar to what we do in Katello (https://github.com/Katello/katello/blob/master/test/katello_test_runner.rb). Minitest finds all methods for a given suite by simply doing:

all_test_methods = suite.send "#{type}_methods"

[GregSutcliffe]

@ehelms awesome, thanks for that. I've updated to use a custom test runner, which solves point (2) - it now works for all test types.

The code is slightly nicer, but we're still replacing the _run_suite method, which is obviously a problem if we move on to another version of Minitest and the code moves on. I'd prefer extending it, but I can't figure out how to mess with filtered_test_methods from outside. Any suggestions?

[GregSutcliffe]

PR Header updated to reflect new code style

[domcleal]

regular merge will do I think?

EDIT: oh, I see what you're trying to do, merging the contents of the arrays? I'm pretty sure deep_merge will only work with hashes. Please add a test to test/unit/plugin_test.rb to cover this.

[GregSutcliffe]

Dang, you're right. Updated, with a test.

[domcleal]

How about using alias_method_chain and then overriding test_methods on suite when it's passed in? Or overriding _run_suites and passing a different suite object into _run_suite which filters them?

[dmitri-d]

I'm not a fan of the idea of disabling tests from a plugin: 1) we are adding test-specific logic into production code; 2) we now have conditionally-run tests, 3) that are being controlled from a plugin.

How often does this happen? The test used as an example in #6549 could be re-written to be less-specific: don't check the number of menu items, but rather that a specific menu item is available.

[ehelms]

@witlessbird what you are saying is that any time a plugin changes a core behavior, they need to update the corresponding core tests to be more generic? Some tests could benefit from that, but I still think plugins should be able to change a behavior and have assurance that other 98% of Foreman tests pass as expected. In the Katello case some tests fail due to difference in workflow (dynflow vs. no dynflow) and some are behavioral such as how we disable nesting:

UserTest#test_0060_return organization and child ids for non-admin user:
Foreman::Exception: ERF42-6434 [Foreman::Exception]: You cannot set an organization's parent_id. This feature is disabled.
    /home/vagrant/katello/app/models/katello/concerns/organization_extensions.rb:268:in `parent_id='

[GregSutcliffe]

@domcleal updated to use method_alias on the suite class and filter out the requested tests. This feels much cleaner.

[domcleal]

Looks fine now, thanks. Could you add a test to exercise the test runner code please? Maybe you can register a test to always skip from our test helper, and have it assert true, "should be skipped due to tests_to_skip" or similar.

Also please update http://projects.theforeman.org/projects/foreman/wiki/How_to_Create_a_Plugin for the API addition, and I think it's done.

[GregSutcliffe]

@domcleal updated with a test which will fail unless the custom runner correctly skips it. Happy to update the docs once this is merged.

[dmitri-d]

Looks good!

[domcleal]

Merged as c474150 for 1.7, thanks @GregSutcliffe.