(MODULES-8348) Refactor the acceptance test scaffold #376

ekinanp · 2019-01-26T04:46:56Z

No description provided.

puppetcla · 2019-01-26T08:00:20Z

CLA signed by all contributors.

ekinanp · 2019-01-28T00:35:08Z

acceptance/tests/test_upgrade_pc1_to_puppet5.rb

 test_name 'puppet_agent class: collection parameter for FOSS upgrades' do
-  master_agent_version = puppet_agent_version_on(master)
+  master_collection restrict_to: 'puppet5'


I think this captures the general pattern of an agent upgrade test based on what I saw from the existing code.

The DSL helper master_collection replaces the existing agent_version checks. Reason I chose collection is b/c it is more consistent with how we think about upgrades.

is_foss_upgrade_test replaces the existing PE_ONLY_UPGRADES confine. Once we start testing PE upgrades here, we may likely have a similar is_pe_upgrade_test helper.

setup_agents_to_upgrade is essentially prepare_upgrade_with but there are some caveats:

The method takes in an optional block for additional set-up. For example on a Windows upgrade test, you can do something like

setup_agents_to_upgrade('pc1') do <start services here> end

Any subsequent teardown blocks after the invocation of setup_agents_to_upgrade cannot use puppet, facter, i.e. anything from the puppet_agent package. For example, something like

setup_agents_to_upgrade('pc1') teardown { on(agent, puppet('--version')) }

will fail because setup_agents_to_upgrade has some teardown blocks to clean the agents so that by the time we get to the declared teardown block, puppet-agent will have already been uninstalled. However, something like

setup_agents_to_upgrade('pc1') on(agent, puppet('--version'))

will work. So the thing to remember here is don't use puppet-agent stuff in any teardowns after setup_agents_to_upgrade.

(There's a technical reason why I moved purge_agents' code over to setup_agents_to_upgrade. See #376 (comment) for the details).

Finally, apply_puppet_agent_class and assert_successful_upgrade are self-explanatory. Their separation makes it easier to test failed upgrade cases in the future (for example, that the pre-upgrade state is restored for failed Windows upgrades).

ekinanp · 2019-01-28T00:47:55Z

acceptance/helpers.rb

      #
      # @param [String] initial_package_version_or_collection Either a version
      #   of puppet-agent or the name of a puppet collection to install the agent from.
-      def prepare_upgrade_with(initial_package_version_or_collection)
+      # @yield Invokes any additional set-up that's required. 
+      def setup_agents_to_upgrade(initial_package_version_or_collection)


The reason why purge_agents' code was moved here and why this method now takes a block is because that is the best solution I could come up with that would ensure a clean SUT between each test, including failed tests. The latter was a big pain point for me while working on this PR because I'd have to (manually) clean out my agent every time my acceptance tests failed (re-running the rake prepare task took a while).

Here were some of the other solutions I thought of. The existing way

setup_agents_to_upgrade('pc1') teardown { purge_agents }

wouldn't work if one needed to do additional preparation before an upgrade. For example, given something like

setup_agents_to_upgrade('pc1') // additonal prep here teardown { purge_agents }

, if anything in the additional prep part of the code threw an exception, then the purge_agents teardown would not be included in Beaker's clean-up, so you'd need to manually go and clean your SUTs.

Something like

begin setup_agents_to_upgrade('pc1') // additional prep here ensure teardown { purge_agents } end

is also bad because (1) you need to remember to do that for every agent upgrade test to ensure a clean SUT and (2), if something in setup_agents_to_upgrade throws an exception, purge_agents will be called on agents that aren't properly configured (the latter happened to me and it was annoying).

The current solution of

setup_agents_to_upgrade('pc1') do // additional prep here end

is also not great because you need to remember to declare your prep. in a block, but outside of that one thing, you can code your prep. anyway you like since any teardowns inside the block are applied before the code in purge_agents (so those teardowns can use things like puppet, facter, etc.).

Is it possible to include the teardown inside the setup_agents_to_upgrade function? https://github.com/puppetlabs/beaker/blob/a01b5732a7d6cec146bd18d08795c64aafe1d2f1/docs/concepts/style_guide.md#teardowns indicates there's no reason we need to wait until after the function completes to specify the teardown.

Then maybe a check in purge_agents to identify if there's an agent installed and skip the teardown if there isn't one?

Addressed this in HipChat. Reason we need the purge_agents teardown at the end of the function is so that something like

setup_agents_to_upgrade('pc1') do <manipulate some state with Puppet> teardown { <restore that state with puppet> } end

would work. Otherwise, when the "restore state" teardown is reached, the agent will have already been uninstalled so the teardown would raise an exception

ekinanp · 2019-01-28T01:02:16Z

acceptance/helpers.rb

-                'daemonize'     => true,
-            }}
-
-            with_puppet_running_on(master, master_conf) do


Another thing I noticed was that with_puppet_running_on spent a lot of time restarting puppetserver and it also cluttered the output when BEAKER_debug was set. This happened at least twice in each test.

I'm not sure why we need to change anything in puppetserver for the agent upgrade tests which is why I removed the uses of with_puppet_running_on here (I re-ran the tests multiple times to make sure, and they all passed). If there is a good reason to keep with_puppet_running_on, then its invocation should be optional.

Do you have to restart puppetserver after clearing out old certs? I don't actually know if that's the case but if it is we need to ensure that's happening.

Asked in the server room just now and it looks like the answer is "no".

ekinanp · 2019-01-28T17:59:39Z

acceptance/helpers.rb

+      # you could accidentally filter out your master and thus raise an exception here.
+      #
+      # @param [Hash] args The specified restriction
+      def master_collection(args)


I'm happy to remove this too if it is a bit of a "too much too early" type of thing.

ekinanp · 2019-01-28T18:00:28Z

I'll need to update the README prior to merging this if the changes here are approved.

mcdonaldseanp

A few questions around the changes to teardowns

mcdonaldseanp · 2019-01-28T22:08:03Z

acceptance/helpers.rb

-      # - Allow for assertions inside in a block
+      # @param [Hash] params The class parameters
+      # @return [String] The generated puppet_agent class declaration
+      def class__puppet_agent(params = {})


Nit: Do we lose any clarity around what the manifest should look like by including this helper?

In the original tests seeing the manifest in it's entirety might make it easier for someone new to the codebase to grasp what the manifest would look like. Not a huge deal since the helper just wraps the manifest params in the classname, but if I were new to the module I would still need to look up this helper to find that out.

mcdonaldseanp · 2019-01-28T22:11:54Z

acceptance/helpers.rb

-                'daemonize'     => true,
-            }}
-
-            with_puppet_running_on(master, master_conf) do


Do you have to restart puppetserver after clearing out old certs? I don't actually know if that's the case but if it is we need to ensure that's happening.

mcdonaldseanp · 2019-01-28T22:22:13Z

acceptance/helpers.rb

      #
      # @param [String] initial_package_version_or_collection Either a version
      #   of puppet-agent or the name of a puppet collection to install the agent from.
-      def prepare_upgrade_with(initial_package_version_or_collection)
+      # @yield Invokes any additional set-up that's required. 
+      def setup_agents_to_upgrade(initial_package_version_or_collection)


Is it possible to include the teardown inside the setup_agents_to_upgrade function? https://github.com/puppetlabs/beaker/blob/a01b5732a7d6cec146bd18d08795c64aafe1d2f1/docs/concepts/style_guide.md#teardowns indicates there's no reason we need to wait until after the function completes to specify the teardown.

Then maybe a check in purge_agents to identify if there's an agent installed and skip the teardown if there isn't one?

mcdonaldseanp · 2019-01-28T22:24:32Z

acceptance/helpers.rb

-            teardown do
-              step "restore original manifest" do
+
+            cleanup = lambda do


What was the reason to change the teardowns to lambdas?

This is so we'd reset the site.pp file after each method invocation instead of after the test finishes. That way, this function can be re-used many times in the same test.

mcdonaldseanp · 2019-01-28T23:05:06Z

Based on the responses the reasoning behind replacing things with blocks/lambdas seems reasonable.

caseywilliams · 2019-01-28T23:33:34Z

acceptance/tests/test_upgrade_puppet5_to_puppet6.rb

 test_name 'puppet_agent class: collection parameter for FOSS upgrades' do
-  master_agent_version = puppet_agent_version_on(master)
+  master_collection restrict_to: 'puppet5'


There are a few things in these updated tests that look like readability problems to me:

It's not obvious to me from the name that is_foss_ugprade_test is restricting the upgrade from PE platforms -- that name sounds like a boolean check of some kind. I'd prefer something like exclude_pe_upgrade_platforms or similar that describes the action it's doing.

Likewise, I'd prefer a verb-oriented method name to replace master_collection. I'd prefer something like require_master_collection('puppet5'), require_master_collection(max: 'puppet5'), or similar.

It seems like assert_successful_upgrade would be better titled assert_agent_version or similar. A 'successful upgrade' may entail more than just updating the puppet-agent version: we may want to assert that certain services are stopped or running depending on the parameters that were used, too.

This is more of a nitpick, but the words 'setup', 'set-up', and 'set up' are all being used here. 'set up' is the verb, and 'setup' is the noun version -- I'd prefer these to be consistent.

I am not a fan of the lambdas, which are not very ruby-like and unfamiliar to people who are used to writing normal beaker tests already, but it's not a big deal.

Edit: this should probably also reference the original ticket number (MODULES-8348) for this work

Updated the PR to address your concerns. Here are the changes:

Changed is_foss_upgrade_test => exclude_pe_upgrade_platforms

Changed to require_master_collection as you specified -- that is cleaner.

assert_successful_upgrade now takes a block so you can do something like

assert_succesful_upgrade('6.0.0') do |agent| // additional assertions end

Addressed the set-up (verb) vs. setup (noun) inconsistency

Updated the commit to reference MODULES-8348

ekinanp · 2019-01-29T03:45:21Z

puppetlabs/beaker-puppet#104 + a beaker-puppet release will need to happen before this one can go in. Sorry it's late, just now remembered to put up that PR.

caseywilliams · 2019-01-29T21:59:33Z

I've released beaker-puppet 1.16.0 with your changes. I will merge this and update the minimum beaker-puppet version in the acceptance Gemfile separately.

ekinanp force-pushed the acceptance-test-refactor branch 7 times, most recently from 9466c39 to 2ae6cee Compare January 26, 2019 07:16

ekinanp force-pushed the acceptance-test-refactor branch 17 times, most recently from 10f4e43 to 2a22f7f Compare January 27, 2019 23:56

ekinanp commented Jan 28, 2019

View reviewed changes

ekinanp requested review from caseywilliams and mcdonaldseanp January 28, 2019 18:03

ekinanp force-pushed the acceptance-test-refactor branch from 2a22f7f to 438d02c Compare January 28, 2019 18:32

mcdonaldseanp requested changes Jan 28, 2019

View reviewed changes

mcdonaldseanp approved these changes Jan 28, 2019

View reviewed changes

ekinanp force-pushed the acceptance-test-refactor branch from 438d02c to e645a63 Compare January 28, 2019 23:16

caseywilliams reviewed Jan 28, 2019

View reviewed changes

ekinanp force-pushed the acceptance-test-refactor branch from e645a63 to 00158ca Compare January 29, 2019 00:16

ekinanp changed the title ~~(maint) Refactor the acceptance test scaffold~~ (MODULES-8348) Refactor the acceptance test scaffold Jan 29, 2019

ekinanp force-pushed the acceptance-test-refactor branch 4 times, most recently from ed3062d to 6b6826b Compare January 29, 2019 00:38

ekinanp added the don't merge label Jan 29, 2019

ekinanp force-pushed the acceptance-test-refactor branch from 6b6826b to 8027dd9 Compare January 29, 2019 03:50

(MODULES-8348) Refactor the acceptance test scaffold

189f223

ekinanp force-pushed the acceptance-test-refactor branch from 8027dd9 to 189f223 Compare January 29, 2019 06:58

caseywilliams removed the don't merge label Jan 29, 2019

caseywilliams approved these changes Jan 29, 2019

View reviewed changes

caseywilliams merged commit b45f4cb into puppetlabs:master Jan 29, 2019

caseywilliams mentioned this pull request Jan 29, 2019

(maint) Update minimum acceptance beaker-puppet version #377

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(MODULES-8348) Refactor the acceptance test scaffold #376

(MODULES-8348) Refactor the acceptance test scaffold #376

ekinanp commented Jan 26, 2019

puppetcla commented Jan 26, 2019

ekinanp Jan 28, 2019 •

edited

ekinanp Jan 28, 2019 •

edited

ekinanp Jan 28, 2019 •

edited

ekinanp Jan 28, 2019 •

edited

mcdonaldseanp Jan 28, 2019

ekinanp Jan 28, 2019 •

edited

ekinanp Jan 28, 2019 •

edited

mcdonaldseanp Jan 28, 2019

ekinanp Jan 28, 2019

ekinanp Jan 28, 2019

ekinanp commented Jan 28, 2019

mcdonaldseanp left a comment

mcdonaldseanp Jan 28, 2019

mcdonaldseanp Jan 28, 2019

mcdonaldseanp Jan 28, 2019

mcdonaldseanp Jan 28, 2019

ekinanp Jan 28, 2019

mcdonaldseanp commented Jan 28, 2019

caseywilliams Jan 28, 2019 •

edited

ekinanp Jan 29, 2019 •

edited

ekinanp commented Jan 29, 2019

caseywilliams commented Jan 29, 2019

(MODULES-8348) Refactor the acceptance test scaffold #376

(MODULES-8348) Refactor the acceptance test scaffold #376

Conversation

ekinanp commented Jan 26, 2019

puppetcla commented Jan 26, 2019

ekinanp Jan 28, 2019 • edited

Choose a reason for hiding this comment

ekinanp Jan 28, 2019 • edited

Choose a reason for hiding this comment

ekinanp Jan 28, 2019 • edited

Choose a reason for hiding this comment

ekinanp Jan 28, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ekinanp Jan 28, 2019 • edited

Choose a reason for hiding this comment

ekinanp Jan 28, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ekinanp commented Jan 28, 2019

mcdonaldseanp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcdonaldseanp commented Jan 28, 2019

caseywilliams Jan 28, 2019 • edited

Choose a reason for hiding this comment

ekinanp Jan 29, 2019 • edited

Choose a reason for hiding this comment

ekinanp commented Jan 29, 2019

caseywilliams commented Jan 29, 2019

ekinanp Jan 28, 2019 •

edited

ekinanp Jan 28, 2019 •

edited

ekinanp Jan 28, 2019 •

edited

ekinanp Jan 28, 2019 •

edited

ekinanp Jan 28, 2019 •

edited

ekinanp Jan 28, 2019 •

edited

caseywilliams Jan 28, 2019 •

edited

ekinanp Jan 29, 2019 •

edited