Skip to content

test: modernize the RSpec suite and cover the workflow logic - #44

Merged
tas50 merged 3 commits into
mainfrom
test/modernize-rspec-suite
Aug 30, 2026
Merged

tas50 merged 3 commits into
mainfrom
test/modernize-rspec-suite

Conversation

@tas50

@tas50 tas50 commented Aug 30, 2026

Copy link
Copy Markdown
Member

What

spec/spec_helper.rb was an empty license header — no requires, no RSpec.configure, and nothing in .rspec to load it. Every spec file had to require its own dependencies, and none of the guardrails that catch stale tests were on.

This wires the suite up the way the rest of our drivers do:

  • .rspec passes --require spec_helper, so the helper is the single place that loads the driver and its Test Kitchen dummies.
  • spec_helper turns on verify_partial_doubles, disable_monkey_patching!, expect-only syntax, raise_errors_for_deprecations!, and random ordering with a printed seed.
  • spec/support/vro_doubles.rb builds verifying doubles for the slice of the vcoworkflows API this driver touches. If vcoworkflows renames token or drops output_parameters, the suite fails instead of passing against a stub that no longer matches the real object.
  • Specs are no longer excluded from cookstyle, so they are held to the same style as lib/.

Coverage

The old suite checked #create and #destroy by stubbing the driver's own methods and asserting they were called. That proves the methods call each other; it proves nothing about what we actually send to vRO. There are now end-to-end examples that stub only at the VcoWorkflows::Workflow boundary and assert the real behaviour: which workflow gets looked up, that configured create and destroy parameters are pushed to it, that server_id reaches the destroy workflow, and that create output lands in instance state.

New coverage aimed at the branches where bugs actually hide:

  • the poll loop in #wait_for_workflow — how many times the token is re-read, that it sleeps two seconds between polls, that it does not sleep when the run has already finished, and that a real elapsed timeout fires
  • request_timeout showing up in the timeout message when it is not the default
  • #set_workflow_parameters stringifying symbol keys, passing non-string values through, and no-opping on an empty hash
  • #output_parameter_value stringifying numbers and unset values
  • vro_disable_ssl_verify reaching VcoWorkflows::Config
  • #execute_create_workflow neither validating output nor writing state when the run failed
  • memoization of vro_config, vro_client, and output_parameters

Also drops a Struct.new assigned to a top-level constant inside an example — it leaked a constant into the whole suite and warned on redefinition — and corrects the spec path in CONTRIBUTING.md, which pointed at a file that does not exist.

No change to lib/. 51 examples became 77.

Verification

$ bundle exec cookstyle --chefstyle
Inspecting 8 files
........

8 files inspected, no offenses detected

$ bundle exec rake test
77 examples, 0 failures

The spec_helper was an empty license header: no requires, no RSpec
configuration, and nothing in `.rspec` to load it. Every spec file had to
require its own dependencies, and none of the guardrails that catch stale
tests were switched on.

This wires the suite up properly:

- `.rspec` now passes `--require spec_helper`, so the helper is the single
  place that loads the driver and its Test Kitchen dummies.
- `spec_helper` turns on `verify_partial_doubles`, `disable_monkey_patching!`,
  expect-only syntax, `raise_errors_for_deprecations!`, and random ordering
  with a printed seed.
- `spec/support/vro_doubles.rb` builds verifying doubles for the slice of the
  vcoworkflows API the driver touches, so a rename upstream fails the suite
  instead of passing against a stub that no longer matches.
- Specs are no longer excluded from cookstyle, so they are held to the same
  style as `lib/`.

It also fills in the logic the old suite left uncovered. Previously `#create`
and `#destroy` were only checked by stubbing the driver's own methods and
asserting they were called, which proves nothing about what the driver
actually sends to vRO. There are now end-to-end examples that stub only at the
`VcoWorkflows::Workflow` boundary and assert the real behaviour: which
workflow gets looked up, that configured create and destroy parameters are
pushed to it, that `server_id` reaches the destroy workflow, and that the
create output lands in instance state.

New coverage for the branches where bugs can hide:

- the poll loop in `#wait_for_workflow` -- how many times the token is
  re-read, that it sleeps two seconds between polls, that it does not sleep
  when the run is already finished, and that a real elapsed timeout fires
- `request_timeout` appearing in the timeout message when it is not the default
- `#set_workflow_parameters` stringifying symbol keys, passing non-string
  values through, and no-opping on an empty hash
- `#output_parameter_value` stringifying numbers and unset values
- `vro_disable_ssl_verify` reaching `VcoWorkflows::Config`
- `#execute_create_workflow` not validating or writing state when the run failed
- memoization of `vro_config`, `vro_client`, and `output_parameters`

Also removes a `Struct.new` assigned to a top-level constant inside an
example, which leaked a constant into the whole suite and warned on redefine,
and corrects the spec path in CONTRIBUTING.md.

51 examples became 77.

Signed-off-by: Tim Smith <tim@mondoo.com>
webmock has never been required or used by anything in spec/ -- the specs
stub the vcoworkflows client rather than the HTTP layer underneath it, so
nothing ever reaches a socket for webmock to intercept.

Removing it also clears the one thing standing between this repo and a green
lint run: cookstyle 9.0.0 flags the extra spacing in that Gemfile line under
Layout/ExtraSpacing, which cookstyle 8 did not, so `main` went red on its own
the moment cookstyle 9 shipped.

Signed-off-by: Tim Smith <tim@mondoo.com>
@tas50

tas50 commented Aug 30, 2026

Copy link
Copy Markdown
Member Author

Pushed an extra commit dropping the unused webmock dev dependency. It was never required by anything in spec/, and cookstyle 9.0.0 (released since the last merge here) flags the extra spacing on that Gemfile line under Layout/ExtraSpacing — so main went red on its own the moment cookstyle 9 shipped, and every PR inherits it. Same one-line commit is on #46 so both can go green independently of merge order.

…res (#45)

A handful of related problems in the create/destroy lifecycle.

`destroy` never removed `server_id` and `hostname` from the state hash. Test
Kitchen writes state back in an `ensure` block even when an action fails, so
when `wait_for_server` gives up and tears the machine down, the state file is
still left naming a server that no longer exists. The next `kitchen destroy`
then runs the destroy workflow a second time against a machine that is
already gone. `destroy` now clears both keys once the workflow succeeds,
which also makes it a no-op the second time it is called in a process.

`wait_for_server` replaced the connection failure with whatever the cleanup
raised. If the machine was unreachable *and* the destroy workflow then failed,
the user saw the vRO error and no hint of why the machine was being destroyed
in the first place. The cleanup failure is now logged -- including a warning
that the server may have been left behind -- and the original connection
failure is what propagates.

`set_workflow_vars` cleared the memoized client but not the memoized output
parameters, so anything reading `output_parameters` after switching from the
create workflow to the destroy workflow would still be looking at the create
workflow's output.

`output_parameter_value` raised `NoMethodError` on nil for any parameter the
workflow did not return, which made `output_parameter_empty?` blow up instead
of answering the question it was asked. It now returns an empty string for a
missing parameter. That in turn removes a dead branch: `output_parameter_empty?`
tested `.nil?` on the result of a `.to_s`, which can never be nil, and
computed the value twice to do it.

Also requires `timeout` explicitly rather than relying on a dependency having
pulled it in, and updates a `rubocop:disable` comment that still used the old
`Style/AccessorMethodName` namespace -- cookstyle printed a deprecation
warning about it on every run.

Signed-off-by: Tim Smith <tim@mondoo.com>
@tas50
tas50 merged commit 1f48161 into main Aug 30, 2026
3 checks passed
@tas50
tas50 deleted the test/modernize-rspec-suite branch August 30, 2026 02:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant