(QENG-3370) Add pe_* value CLI override options. #21

waynr · 2016-01-26T18:09:05Z

No description provided.

waynr · 2016-01-26T18:10:03Z

@doug-rosser how does this look?

Also, please note that this patch still needs tests added.

…t class.

This may be necessary to ensure coverage checks can work correctly. Also, it makes sense to test at this since I know of at least one project that is doing something similar. Not so sure about the way this is handling stderr though--just trying to avoid polluting the minitest console output.

This script allows one to generate test fixtures using beaker-hostgenerator itself. Yay. The generated directory structure could use some more thought.

This will make it easier to generate many many fixtures.

Unclutters the fixture generator class and maintains distinction between test and helper methods.

…ost roles.

rick · 2016-02-01T16:51:40Z

test/test_generator.rb

+      end
+
+      fixture_file = File.open(f, "r")
+      fixture_hash = YAML.load(fixture_file)


Idiomatically, I would probably drop the temp file variable and go with:

fixture_hash = YAML.load_file(f)

or

fixture_hash = YAML.load(File.read(f))

(The "or" because I can't remember if load_file is always available, may depend on YAML version or implementation).

rick · 2016-02-01T16:53:23Z

OK, so the fixtures are used to snapshot the behavior of the generator at this point in time?

rick · 2016-02-01T16:56:45Z

test/test_generator.rb

+      options = fixture_hash["options_string"]
+      options = options.split
+      test_hash = run_cli_with_options(options)
+      assert_equal(fixture_hash["expected_hash"], test_hash)


This is probably a good place to document (for instance via the 3rd argument to assert_equal) what this failure means. I believe it means "the behavior of the generator has changed from the snapshotted behavior".

If that's true folks are also going to want guidance on how/when to re-snapshot things so that they can take the changed behavior as a new baseline.

* Move FixtureGenerator class into test/utils/generator_helpers.rb * Create generate:test Rake task

waynr · 2016-02-01T19:20:16Z

@rick yeah, snapshotting is a good way to describe it. I think I've addressed all your comments. I added a generate:fixtures rake task and added an assert_equal mismatch helper message indicating which fixture borked and the options passed to beaker-hostgenerator.

Unfortunately the tests stop running on the first assertion failure so I'm still not sure this is a particularly good way to write the test_fixtures function. It'd be nice to see all the broken fixtures for a given beaker-hostgenerator change at once.

This makes it possible to treat each individual fixture its own test case which improves the reporting and ensures that all fixtures are tested even if some fail. Using a loop containing an assertion in minitest did not work for this purpose because a failed assertion caused the loop to break prematurely; it probably would have been possible to catch whatever the exception was and continue the loop but whatever. I already did this.

doug-rosser · 2016-02-03T00:56:35Z

@waynr The code looks good, but the old (weird) behavior is not present. Even though I think the usefulness is dubious, the old code (master branch) would set pe_dir if pe_family and pe_version were the same string. This new code doesn't preserve that behavior. Example:

[beaker-hostgenerator (master)]$ env | grep pe_
pe_family=2016.1.0
pe_version=2016.1.0
[beaker-hostgenerator (master)]$ bin/beaker-hostgenerator centos6-64mdca
WARNING: Starting with beaker-hostgenerator 1.x platform strings for "el" hosts
will correspond to the actual linux distribution name. ie, the platform string
corresponding to a host specified as "centos4_64a" will be "centos-4-x86_64"
rather than "el-4-x86_64". It is recommended that you update your project's test
suites ASAP or be forced to do so when beaker-hostgenerator development moves on
to the 1.x series. We don't intend to backport features or platforms to 0.x.
---
HOSTS:
  centos6-64-1:
    pe_dir: http://enterprise.delivery.puppetlabs.net/archives/releases/2016.1.0/
    pe_ver: 2016.1.0
    pe_upgrade_dir: 
    pe_upgrade_ver: 
    hypervisor: vmpooler
    platform: el-6-x86_64
    template: centos-6-x86_64
    roles:
    - agent
    - master
    - database
    - dashboard
CONFIG:
  nfs_server: none
  consoleport: 443
  pooling_api: http://vmpooler.delivery.puppetlabs.net/
[beaker-hostgenerator (master)]$ 
[beaker-hostgenerator (master)]$ git checkout qeng-3370 
Switched to branch 'qeng-3370'
Your branch is up-to-date with 'upstream/qeng-3370'.
[beaker-hostgenerator (qeng-3370)]$ 
[beaker-hostgenerator (qeng-3370)]$ bin/beaker-hostgenerator centos6-64mdca
WARNING: Starting with beaker-hostgenerator 1.x platform strings for "el" hosts
will correspond to the actual linux distribution name. ie, the platform string
corresponding to a host specified as "centos4_64a" will be "centos-4-x86_64"
rather than "el-4-x86_64". It is recommended that you update your project's test
suites ASAP or be forced to do so when beaker-hostgenerator development moves on
to the 1.x series. We don't intend to backport features or platforms to 0.x.
---
HOSTS:
  centos6-64-1:
    pe_dir: 
    pe_ver: 
    pe_upgrade_dir: 
    pe_upgrade_ver: 
    hypervisor: vmpooler
    platform: el-6-x86_64
    template: centos-6-x86_64
    roles:
    - agent
    - master
    - database
    - dashboard
CONFIG:
  nfs_server: none
  consoleport: 443
  pooling_api: http://vmpooler.delivery.puppetlabs.net/

Since the OSINFO and OSINFO_BHGv1 module constants are only initialized once (during library load), do not refer directly to them using reference variables. Instead, initialize the osinfo variable directly on each call of get_osinfo and merge in the module constants as appropriate based on the given beaker-hostenerator version integer. This bug never came up during normal command-line usage of beaker-hostgenerator because get_osinfo would never be expected to be called with both bhg_version=0 and bhg_version=1. However, it came up both during generation of test fixtures and validation of those test fixtures in rspec since the module is only loaded once which led to the mutated value of the OSINFO module constant to persist across all initializations of the BeakerHostGenerator::CLI class.

Similar to the BeakerHostGenerator::Data::Vmpooler data initialization bug, this is something that would not typically show up during normal command line usage of beaker-hostgenerator because at that time there is typically only one reference made to the BeakerHostGenerator::Utils module constant. However during test fixture generation and rspec tests the modules were only loaded once so there was only one attempt to read in environment variables which led to inconsisten results depending on the order in which the fixtures were generated/tested.

waynr · 2016-02-03T19:49:10Z

@doug-rosser okay, I've fixed that bug. The problem was here: aa3e08c#diff-0623c137fe4358c047889a4ff1adc01bL62

There were some other module constant initialization problems throughout the beaker-hostgenerator library that were leading to incorrectly-generated fixtures which have now been fixed.

doug-rosser · 2016-02-03T22:25:43Z

👍 👍 👍

nwolfe · 2016-02-05T02:04:26Z

.projectile

@@ -0,0 +1 @@
+-/vendor


Do we want to commit an emacs file like this?

I added it because I think of it as similar to a .gitignore file which can often contain subjectively-named file paths. e.g. https://github.com/puppetlabs/beaker-hostgenerator/blob/master/.gitignore#L13-L14 (That .gitignore originally came from puppetlabs/beaker-template)

The difference here is that I don't care as much about .gitignore so I never bothered to add vendor to it the way I do for projectile. Local package installation is much more annoying in projectile than in git.

I would defer to you as far as whether you think it's a good idea or not--I haven't really thought through the implications for other emacs users. I'm sure there is some way to configure projectile exclude paths in one's emacs configuration which would addres my problem more generally.

I think you want to add .projectile to your global ~/.gitignore, and then you can sprinkle these projectile ignore files anywhere.

Alternately, you could just configure projectile to globally ignore vendor directories with projectile-globally-ignored-directories if that suits you better.

Either way I wouldn't think we'd want to start committing editor-specific files.

Are you saying that I should remove this file from the PR? I don't have a problem with that, but just to clarify I intentially added it in the first place.

Ha yeah I'd say remove it from the PR and achieve what you're trying to do using one of the options I mentioned above.

But this is a good idea, I'm going to ignore vendor directories too 😄

This reverts commit 3dd4eb3.

rick · 2016-02-05T16:31:32Z

README.md

+`generate:fixtures` task:
+
+- When you modify the `FixtureGenerator` to generate new fixtures.
+- When you need to fix bug (generated hosts are not usable without your change,


💄 "fix a bug"

nwolfe · 2016-02-05T17:40:21Z

Gemfile

@@ -21,6 +21,8 @@ group :acceptance_testing do
  gem "beaker", *location_for(ENV['BEAKER_VERSION'] || '~> 2.0')
 end

+gem "minitest"


Should this be under a test or development group so it's not required for normal usage?

nwolfe · 2016-02-05T17:48:15Z

👍 - left some minor nitpicks

er0ck · 2016-02-05T20:01:01Z

Rakefile

 # namespace-named default tasks.
 # these are the default tasks invoked when only the namespace is referenced.
 # they're needed because `task :default` in those blocks doesn't work as expected.
-task 'test:spec' => 'test:spec:run'
+task 'test:spec' => ['test:spec:run', 'test:spec:minitest']


shouldn't :generate:fixtures be a dependency of this task?

No, see the documentation at:
https://github.com/puppetlabs/beaker-hostgenerator/blob/qeng-3370/README.md#generated-fixtures

The fixture generator is not meant to be run every time the tests are run--that's why the fixtures are committed to the repo, to "snapshot" the current behavior of beaker-hostgenerator in a way that helps prevent regressions as we refactor or repurpose parts of the code.

Anyway, we generated fixtures before every time we run the specs that use those fixtures that spec would always pass unless we do something complicated to generate the fixtures for some older commit of the repo then test using them against the HEAD of the PR branch (which would assume a certain CI setup).

justinstoller · 2016-02-05T23:12:26Z

lib/beaker-hostgenerator/data.rb

-    PE_UPGRADE_VERSION=ENV['pe_upgrade_version']
-    PE_UPGRADE_FAMILY=ENV['pe_upgrade_family']
+    def pe_version
+      ENV['pe_version']


Do we want to memoize calls to the environment here or do we expect these values to change?

We expect these values to change in the context of both test fixture generation and test fixture validation:
https://github.com/puppetlabs/beaker-hostgenerator/blob/qeng-3370/spec/beaker-hostgenerator/generator_spec.rb#L106-L113
https://github.com/puppetlabs/beaker-hostgenerator/blob/qeng-3370/test/util/generator_helpers.rb#L34-L40

Otherwise I probably would probably hate Ruby slightly less right now.

justinstoller · 2016-02-06T00:06:48Z

In general this looks like a good change and it has great documentation and automation around it.

However, I usually consider fixture/data driven testing like this an anti-pattern. I think it has a valuable place when you have legacy behavior around spaghetti code that is hard to test otherwise, or you have folks writing tests that may not be familiar with the code under test and/or development in general. I would prefer testing the logic in the code not the variations in the data.

Having said that, that's the only thing I have an issue with, and it would appear from the previous comments that I'm in the minority, just felt obligated to mention it.

All in all, 👍

(QENG-3370) Add pe_* value CLI override options.

er0ck · 2016-02-06T00:25:30Z

justin has a good point about what appears to be feature level testing. i'm okay with that. maybe should move to an integration suite, and unit tests should be added for unit/class/module logic

This should have been changed in commit c6507fd (voxpupuli#21).

waynr force-pushed the qeng-3370 branch from f028a93 to 6e019f4 Compare January 26, 2016 18:09

Wayne added 4 commits January 29, 2016 14:22

(QENG-3370) Add pe_* value CLI override options.

e321afc

(QENG-3770) Stub out minitest for Generator class.

3af5796

(QENG-3770) Get test:spec rake task to run minitest also.

436bb77

(QENG-3770) Add GeneratorTestHelpers and default options test for tes…

388867d

…t class.

waynr force-pushed the qeng-3370 branch from 6e019f4 to 388867d Compare January 29, 2016 23:28

Wayne Warren added 8 commits January 31, 2016 09:23

(QENG-3370) Move GeneratorTestHelpers into its own file.

f7d3f99

(QENG-3370) Add janky fixture generator.

88ac034

This script allows one to generate test fixtures using beaker-hostgenerator itself. Yay. The generated directory structure could use some more thought.

(QENG-3370) Create common method for generating options-based fixtures.

6be93dd

This will make it easier to generate many many fixtures.

(QENG-3370) Move helper methods into helper module.

f4fd1cf

Unclutters the fixture generator class and maintains distinction between test and helper methods.

(QENG-3370) Use enumerators to produce more diverse combinations of h…

2f913ae

…ost roles.

(QENG-3370) Generate various option fixtures.

71e4efc

(QENG-3370) Add some fixtures and a test case to iterate over them.

722abcd

rick reviewed Feb 1, 2016
View reviewed changes

Wayne added 7 commits February 1, 2016 09:12

(QENG-3370) Don't use minitest to generate fixtures.

6c3a890

(QENG-3370) Use YAML.load_file method.

0b35398

(QENG-3370) Add .projectile (emacs plugin) path exclusion.

3dd4eb3

(QENG-3370) Add assert_equal mismatch message.

11cbba2

(QENG-3370) Make it a little easier to generate fixtures.

a829a42

* Move FixtureGenerator class into test/utils/generator_helpers.rb * Create generate:test Rake task

(QENG-3370) Add documentation for test fixtures.

b6712a9

(QENG-3370) Rename options_string to arguments_string.

c6507fd

waynr force-pushed the qeng-3370 branch from 6f0fdb0 to b401574 Compare February 2, 2016 14:02

(QENG-3370) Minor cleanup of new rspec test.

e9a77bb

Wayne added 2 commits February 3, 2016 11:46

Wayne added 2 commits February 3, 2016 11:51

Merge remote-tracking branch 'origin/master' into qeng-3370

8559505

(QENG-3370) Fix fixtures after merging cisco changes from master branch.

e1c65ff

nwolfe reviewed Feb 5, 2016
View reviewed changes

Revert "(QENG-3370) Add .projectile (emacs plugin) path exclusion."

1db7dfc

This reverts commit 3dd4eb3.

rick reviewed Feb 5, 2016
View reviewed changes

(QENG-3370) Fix grammar no no.

4d7d5db

nwolfe reviewed Feb 5, 2016
View reviewed changes

(QENG-3370) Address PR feedback.

19cc3a9

er0ck reviewed Feb 5, 2016
View reviewed changes

(QENG-3370) Use mixins instead of module method references.

eecc04c

justinstoller reviewed Feb 5, 2016
View reviewed changes

nwolfe added a commit that referenced this pull request Feb 6, 2016

Merge pull request #21 from puppetlabs/qeng-3370

45bf229

(QENG-3370) Add pe_* value CLI override options.

nwolfe merged commit 45bf229 into master Feb 6, 2016

waynr deleted the qeng-3370 branch February 10, 2016 21:55

kenyon added a commit to kenyon/beaker-hostgenerator that referenced this pull request Nov 27, 2023

README: options_string was renamed to arguments_string

c9c47b8

This should have been changed in commit c6507fd (voxpupuli#21).

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(QENG-3370) Add pe_* value CLI override options. #21

(QENG-3370) Add pe_* value CLI override options. #21

waynr commented Jan 26, 2016

waynr commented Jan 26, 2016

rick Feb 1, 2016

rick commented Feb 1, 2016

rick Feb 1, 2016

waynr commented Feb 1, 2016

doug-rosser commented Feb 3, 2016

waynr commented Feb 3, 2016

doug-rosser commented Feb 3, 2016

nwolfe Feb 5, 2016

waynr Feb 5, 2016

waynr Feb 5, 2016

nwolfe Feb 5, 2016

nwolfe Feb 5, 2016

waynr Feb 5, 2016

nwolfe Feb 5, 2016

rick Feb 5, 2016

waynr Feb 5, 2016

nwolfe Feb 5, 2016

nwolfe commented Feb 5, 2016

er0ck Feb 5, 2016

waynr Feb 5, 2016

justinstoller Feb 5, 2016

waynr Feb 5, 2016

justinstoller commented Feb 6, 2016

er0ck commented Feb 6, 2016

		@@ -0,0 +1 @@
		-/vendor

(QENG-3370) Add pe_* value CLI override options. #21

(QENG-3370) Add pe_* value CLI override options. #21

Conversation

waynr commented Jan 26, 2016

waynr commented Jan 26, 2016

Choose a reason for hiding this comment

rick commented Feb 1, 2016

Choose a reason for hiding this comment

waynr commented Feb 1, 2016

doug-rosser commented Feb 3, 2016

waynr commented Feb 3, 2016

doug-rosser commented Feb 3, 2016

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

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

nwolfe commented Feb 5, 2016

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

justinstoller commented Feb 6, 2016

er0ck commented Feb 6, 2016