burtlo/cleaner cli formatter #1331

burtlo · 2016-11-29T01:20:47Z

Updates RSpec CLI Formater to print profiles correctly

The profiles will display the controls with their results and then display the examples not associated with any control but within the profile.

Profile: InSpec Profile (bananas_in_pajamas)
Version: 0.1.0
Target:  local://

  ✖  ssh (b in p): A human-readable title (1 failed)
     ✔  File sandia/bananas_in_pajamas should exist
     ✖  System Package ssh should be installed
     expected that `System Package ssh` is installed
  ✔  Not Running a Web Server (b in p): A human-readable title
     ✔  File /tmp should be directory
     ✔  System Package httpd should not be installed
     ✔  Service httpd should not be running
  ✔  sftp (b in p): A human-readable title
     ✔  System Package sftp should not be installed

  File file_in_profile_between_controls
     ✖  should exist
     expected File file_in_profile_between_controls to exist
     ✔  should not be owned by "root"
  File file_in_profile_outside_of_control
     ✔  should not exist

Profile: Broken Arrow Profile (broken_arrow)
Version: 1.0.0
Target:  local://

  ✖  CHEF FILES (broken_arrow): Chef files are important to have in place (expected File broken_arrow to exist)
     ✖  File broken_arrow should exist
     expected File broken_arrow to exist

  Service broken_arrow
     ✔  should not be running

Profile: InSpec Profile (my_blank)
Version: 0.1.0
Target:  local://


  File profile_with_a_control_no_examples
     ✔  should not exist

Target:  local://

  ✖  Control (no profile): Hello World (expected nil to match "Hello World")
     ✖  File control_outside.txt content should match "Hello World"
     expected nil to match "Hello World"


Profile Summary: 4 successful, 4 failures, 0 skipped
Test Summary: 17 successful, 6 failures, 0 skipped

burtlo · 2016-11-29T01:22:45Z

I would like to also break these formatters out into separate files inside a formatters directory. But let's start with this large file and this huge change.

burtlo · 2016-11-29T01:27:55Z

I tried to clean up the entire CLI formatter by addressing what was happening in #format_example and #close. You will find the logic there is easier to understand as I opted for long method names that describe what I am trying to convey.

From there I wrap the control data into a class I call Control which allows me to make it behave a little more sanely throughout the rest of the methods.

This is still pretty tricky code because you have:

When one example fails for an example you want the control to report as failed which means you can't simply print them right away and instead have to wait until #close or looking at a new example for another control
Anonymous examples (examples not in a control inside of a profile) are collected up and displayed at the end of a profile which means you have to collect them and then display them before switching profiles or at #close

burtlo · 2016-11-29T02:57:54Z

lib/inspec/rspec_json_formatter.rb

@@ -63,7 +63,7 @@ def stop(notification)
  private

  def format_example(example)
-    if example.metadata[:description_args].length > 0 && !example.metadata[:skip].nil?
+    if !example.metadata[:description_args].empty? && example.metadata[:skip]


The first statement was a suggestion by Rubocop. The second is the same way of expressing the same thing.

Agreed on the first. For the second my personal opinion has always been to prefer clarity. It does require one more step in thinking. overall ok :)

burtlo · 2016-11-29T02:58:17Z

lib/inspec/rspec_json_formatter.rb

@@ -92,7 +92,7 @@ def format_example(example)
 end

 class InspecRspecJson < InspecRspecMiniJson # rubocop:disable Metrics/ClassLength
-  RSpec::Core::Formatters.register self, :start, :stop, :dump_summary
+  RSpec::Core::Formatters.register self, :stop, :dump_summary


I refactored the @profiles_info to a memozied helper method so I no longer need to use #start.

burtlo · 2016-11-29T05:08:58Z

lib/inspec/rspec_json_formatter.rb

+  # This class wraps a control hash object to provide a useful inteface for
+  # maintaining the associated profile, ids, results, title, etc.
+  #
+  class Control


Don't love that I used the name Control here but it is one within this context. I wouldn't want to confuse anyone here.

chris-rock · 2016-11-29T10:12:07Z

@burtlo This is changing a lot. Thank you for the hard work. Some comments I have:

lets rebase to latest master (that would resolve the conflict)
does this mean WIP: Vj/clean up formatter #1315 is obsolete now?
lets add functional tests to reflect that behavior (reuse profiles from https://github.com/chef/inspec/tree/master/test/unit/mock/profiles or add new ones)

burtlo · 2016-11-29T15:25:00Z

@vjeffrey did the initial work during our hack day and then I jumped on board. When I pushed my latest changes on my branch I failed to find that pull request again and get that integrated. So yes this appends more changes on top of her work.

@chris-rock I'll add the tests today and continue to rebase this branch.

chris-rock · 2016-11-29T16:00:33Z

@burtlo Just rebase it on latest master, this enables us to see your individual contributions.

burtlo · 2016-11-30T06:56:55Z

This is the current state of rake test:functional on master:

130 runs, 275 assertions, 56 failures, 0 errors, 0 skips

You may need to disregard my results here. I noticed that a lot of the tests are failing because I have some noisy gems.

-""
+"WARN: Unresolved specs during Gem::Specification.reset:
+      ffi (>= 1.0.1)
+      multi_json (~> 1.10)
+WARN: Clearing out unresolved specs.
+Please report a bug if this causes problems.
+"

I need to run bundle exec rake test:functional.

I found a number of issues that I have addressed. Thank goodness for test suites.

burtlo · 2016-11-30T09:31:34Z

Given a control defined like this:

control "tmp-1.0" do                        # A unique ID for this control
  impact 0.7                                # The criticality, if this control fails.
  title "Create /tmp directory"             # A human-readable title
  desc "An optional description..."         # Describe why this is needed
  tag data: "temp data"                     # A tag allows you to associate key information
  tag "security"                            # to the test
  ref "Document A-12", url: 'http://...'    # Additional references

  describe file('/tmp') do                  # The actual test
    it { should be_directory }
    it { should_not be_directory }
  end
end

The results were ordered: fails + skips + passes. Generating the following:

Profile: InSpec example simple inheritance (simple inheritance)
Version: 1.0.0
Target:  local://

  ×  tmp-1.0: Create /tmp directory (1 failed)
     ×  File /tmp should not be directory
     expected `File /tmp.directory?` to return false, got true
     ✔  File /tmp should be directory

My refactored formatter orders based on the examples position within the control. Generating the following:

Profile: InSpec example simple inheritance (simple inheritance)
Version: 1.0.0
Target:  local://

  ×  tmp-1.0: Create /tmp directory (1 failed)
     ✔  File /tmp should be directory
     ×  File /tmp should not be directory
     expected `File /tmp.directory?` to return false, got true

Should I leave it as positioning or have them re-ordered based on previous ordering?

chris-rock · 2016-11-30T10:01:35Z

@burtlo I like the new ordering, it is more natural for the user

burtlo · 2016-11-30T21:17:08Z

This is getting trickier by the minute. What should the output look like when a profile includes another profile like the one defined in simple_inheritance which has a control file defined as:

include_controls 'failures'

describe file('/tmp') do
  it { should be_directory }
end

Because of the way the examples are processed and the way that the profiles are treated the output looks like the following at the moment:

Profile: InSpec example simple inheritance (simple inheritance)
Version: 1.0.0
Target:  local://


  File /tmp
     ✔  should be directory

  ×  tmp-1.0: File /tmp should not be directory (expected `File /tmp.directory?` to return false, got true)
     ×  File /tmp should not be directory
     expected `File /tmp.directory?` to return false, got true
  ✔  cmp-1.0: 7 should cmp == "7"
     ✔  7 should cmp == "7"

The anonymous example is processed first and belongs to the simple_ineritance profile. When the included controls are brought in no profile is added through #add_profile so there is nil for the profile. Which means we are switching profiles which means we need to display all of the anonymous examples and then display all of the new examples without a profile.

I think the best course of action is to see if I can get the profile added to the list of profiles known to the formatter when the include_controls happens.

I assume the desired output would be:

Profile: InSpec example simple inheritance (simple inheritance)
Version: 1.0.0
Target:  local://


  File /tmp
     ✔  should be directory

Profile: failures
Version: 0.1.0
Target: local://

  ×  tmp-1.0: File /tmp should not be directory (expected `File /tmp.directory?` to return false, got true)
     ×  File /tmp should not be directory
     expected `File /tmp.directory?` to return false, got true
  ✔  cmp-1.0: 7 should cmp == "7"
     ✔  7 should cmp == "7"

Which means I need to update lib/inspec/runner.rb to collect all the profiles from the requirements:

    def load
      all_controls = []

      @target_profiles.each do |profile|
        @test_collector.add_profile(profile)
        write_lockfile(profile) if @create_lockfile
        profile.locked_dependencies
        # store the profile_context generated ...
        profile_context = profile.load_libraries
        # so that it can be used here to review the requirements
        profile_context.dependencies.list.values.each do |requirement|
          @test_collector.add_profile(requirement.profile)
        end

        @attributes |= profile.runner_context.attributes
        all_controls += profile.collect_tests
      end

      all_controls.each do |rule|
        register_rule(rule) unless rule.nil?
      end
    end

burtlo · 2016-11-30T22:05:44Z

I'm getting intermittent failures when I run bundle exec rake test:functional. Most of the time everything passes. Occasionally an error or failure.

$ bundle exec rake test:functional
/Users/franklinwebber/inspec/inspec/lib/inspec/dependencies/requirement.rb:3: warning: loading in progress, circular require considered harmful - /Users/franklinwebber/inspec/inspec/lib/inspec/dependencies/dependency_set.rb
# ...
Run options: --seed 63158

# Running:

.................................................................................F................................................

Finished in 76.951614s, 1.6894 runs/s, 6.6665 assertions/s.

  1) Failure:
example inheritance profile#test_0008_use lockfile in tarball [/Users/franklinwebber/inspec/inspec/test/functional/inspec_vendor_test.rb:135]:
Expected: 0
  Actual: 1

130 runs, 513 assertions, 1 failures, 0 errors, 0 skips

$ bundle exec rake test:functional
/Users/franklinwebber/inspec/inspec/lib/inspec/dependencies/requirement.rb:3: warning: loading in progress, circular require considered harmful - /Users/franklinwebber/inspec/inspec/lib/inspec/dependencies/dependency_set.rb
# ...
Run options: --seed 65266

# Running:

..................................................................................................................................

Finished in 79.132946s, 1.6428 runs/s, 6.5207 assertions/s.

130 runs, 516 assertions, 0 failures, 0 errors, 0 skips

$ bundle exec rake test:functional
/Users/franklinwebber/inspec/inspec/lib/inspec/dependencies/requirement.rb:3: warning: loading in progress, circular require considered harmful - /Users/franklinwebber/inspec/inspec/lib/inspec/dependencies/dependency_set.rb
# ...
Run options: --seed 57062

# Running:

.............E....................................................................................................................

Finished in 96.696932s, 1.3444 runs/s, 5.3363 assertions/s.

  1) Error:
inspec exec#test_0001_can generate keys:
Errno::ENOENT: No such file or directory @ unlink_internal - a30878ac-8e99-4b9c-a340-5bf5f13e82a8.pem.pub

burtlo · 2016-11-30T22:13:13Z

lib/inspec/runner.rb

+        profile_context = profile.load_libraries
+
+        profile_context.dependencies.list.values.each do |requirement|
+          @test_collector.add_profile(requirement.profile)


To properly display the dependent profiles I needed to view the ProfileContext and the profiles within it. Is there a better way to grab the profiles that are required?

burtlo · 2016-11-30T22:13:42Z

test/functional/inspec_exec_test.rb

@@ -197,7 +197,7 @@
    let(:out) { inspec('exec ' + simple_inheritance) }

    it 'should print the profile information and then the test results' do
-      out.stdout.force_encoding(Encoding::UTF_8).must_include "local://\n\n\n\e[38;5;9m  ×  tmp-1.0: Create /tmp directory (1 failed)\e[0m\n\e[38;5;9m     ×  File /tmp should not be directory\n"
+      out.stdout.force_encoding(Encoding::UTF_8).must_include "\e[38;5;9m  ×  tmp-1.0: Create /tmp directory (1 failed)\e[0m\n\e[38;5;41m     ✔  File /tmp should be directory\e[0m\n\e[38;5;9m     ×  File /tmp should not be directory\n"


This test was expecting errors + passes + skips. Now the format of the output is in the natural order of the control.

burtlo · 2016-11-30T22:14:31Z

lib/inspec/rspec_json_formatter.rb

@@ -75,6 +75,7 @@ def format_example(example)

    res = {
      id: example.metadata[:id],
+      profile_id: example.metadata[:profile_id],


This makes it easier to get to the id of the profile instead of the previous process of trying to find it.

burtlo · 2016-11-30T22:16:05Z

lib/inspec/rspec_json_formatter.rb

+  class Control
+    include Comparable
+
+    def <=>(other)


Comparable is implemented here not for sorting but for better comparison between controls to ensure that similar named controls in different profiles are considered and displayed separately.

burtlo · 2016-11-30T22:59:51Z

Other than those intermittent errors and failures I feel like this content is tested with the existing tests and ready for some more review.

burtlo · 2016-12-01T00:06:23Z

I've seen this issue locally and it was fixed when I ran it again. https://travis-ci.org/chef/inspec/jobs/180257685#L631

burtlo · 2016-12-07T17:54:06Z

Rebasing off of master this morning to keep it current.

burtlo · 2016-12-12T16:20:13Z

Rebasing off of master this morning to keep it current.

This is a new error that seems related to the output of the formatter: https://travis-ci.org/chef/inspec/jobs/183295983#L686

I will take a look and address the issue.

burtlo · 2016-12-13T17:24:03Z

I've re-run this several times on my local machine, with the seed that showed the issue rake test:functional TESTOPTS="--seed=15875" and other random seeds. They all pass.

Would someone run these tests on their system with this seed or other random seeds and ensure that you see it all working.

BTW I was getting serious errors when initially running the test because of some gems that needed cleaning that were gumming up the very brittle output that we were verifying against:

$ bin/inspec --help
WARN: Unresolved specs during Gem::Specification.reset:
    ffi (some version)
    gem_name (some version)
WARN: Clearing out unresolved specs.
Please report a bug if this causes problems.

So I needed to do some gem cleaning: http://stackoverflow.com/questions/17936340/unresolved-specs-during-gemspecification-reset

arlimus

Overall I love these huge improvements. There is a lot in here and some fantastic changes to readability and flow. Let's get a few comments covered. Huge kudos @burtlo !!

arlimus · 2016-12-14T13:16:35Z

lib/inspec/rspec_json_formatter.rb

@@ -63,7 +63,7 @@ def stop(notification)
  private

  def format_example(example)
-    if example.metadata[:description_args].length > 0 && !example.metadata[:skip].nil?
+    if !example.metadata[:description_args].empty? && example.metadata[:skip]


Agreed on the first. For the second my personal opinion has always been to prefer clarity. It does require one more step in thinking. overall ok :)

arlimus · 2016-12-14T13:39:53Z

lib/inspec/rspec_json_formatter.rb

-    @control_tests.each do |control|
+    all_unique_controls = Array(@all_controls).uniq
+
+    all_unique_controls.each do |control|


just consistency: your next change further down doesn't assign the variable

Array(@all_controls).uniq.each do |control|

pick one 😁 ; variables are great for clarity and re-use, but may not be needed in this case

So true. 👍

arlimus · 2016-12-14T13:42:49Z

lib/inspec/rspec_json_formatter.rb

+
+  def examples_with_controls
+    (examples - examples_without_controls)
+  end


Nice section overall, gz 👍

arlimus · 2016-12-14T14:15:42Z

lib/inspec/rspec_json_formatter.rb

@@ -105,50 +106,36 @@ def initialize(*args)
    @backend = nil
  end

+  attr_reader :profiles


Do we need to expose profiles? If not imho it's great to leave it private. What do you think?

You're right it doesn't need to be public. The methods associated with the interface for formatter and #add_profile are likely the only things that need to be public. Will definitely make the change and make more things private.

arlimus · 2016-12-14T14:17:18Z

lib/inspec/rspec_json_formatter.rb

-  def example2profile(example, profiles)
-    profiles.find { |p| profile_contains_example?(p, example) }
+  def examples
+    @examples ||= @output_hash.delete(:controls)


I really like caching personally. In this example I'm not sure about when it is called. What do you think about assigning examples when they naturally need to be in the code flow?

It is set through in InspecRspecJson#stop when it is called through InspecRspecJson#examples_without_controls / InspecRspecJson#examples_with_controls. I've didn't like the @output_hash.delete(:controls) so I've changed it to retrieve the value from the hash, no longer storing the value. So the method remains as a way to alias of @output_hash[:controls] to examples.

The full JSON formatter was using the start step to setup the profiles_info. I moved that to a memozied method so that the first time it is called it will be created. Signed-off-by: Franklin Webber <franklin@chef.io>

Cleans up the #stop action on the JSON formatter by creating more methods that memoize values or provide values through a method interface. There is still more that can be done with the whole mapping examples to controls through profiles. Signed-off-by: Franklin Webber <franklin@chef.io>

A lot of the work in #flush_current_control is acting on the control. I am starting the flip of the control and bringing those messages being sent originating from a control class itself. Signed-off-by: Franklin Webber <franklin@chef.io>

The profiles will display the controls with their results and then display the examples not associated with any control but within the profile. Signed-off-by: Franklin Webber <franklin@chef.io>

* Fixes an issue when specifying no profile * Fixes an issue when displaying a profile that has included/required profiels * Fixes an issue when specifying profiles with only metadata * Fixes formatting for spacing to ensure it adheres to previous alignment * Fixes issue with the Control object and the rolling up of failed and skipped examples. Signed-off-by: Franklin Webber <franklin@chef.io>

* Moved things around for better understanding of the class * Used `private` to denote what was on the public interface * Solved the ugly TODO which was calculating the state of the control's summary * Used `#examples` instead of `res = control[:results]` throughout the #summary and #title methods Signed-off-by: Franklin Webber <franklin@chef.io>

The class size is too big and Rubocop is right. There are a few more classes in there that could be extracted but I am going to ignore it. The other issues that it presented were fair. Signed-off-by: Franklin Webber <franklin@chef.io>

@arlimus

Based on some feedback from @arlimus there were some methods that were not part of the public inteface that I moved to private. I changed the examples collection from a delete from the output_hash to retrieve the controls. Created a helper for the all_unique_controls which was used in two helper methods. Signed-off-by: Franklin Webber <franklin@chef.io>

@profiles

The profiles method was never public and the @profiles is clearer. Signed-off-by: Franklin Webber <franklin@chef.io>

burtlo · 2016-12-14T19:35:58Z

Rebasing off current chef/inspec:master

arlimus · 2016-12-15T00:31:56Z

Thank you so much for the huge improvement @burtlo and for all the continuous fixes you have added to get this merged!! 🎉 👍 😀

burtlo commented Nov 29, 2016

View reviewed changes

chris-rock mentioned this pull request Nov 29, 2016

WIP: Vj/clean up formatter #1315

Closed

burtlo changed the title ~~Burtlo/cleaner cli formatter~~ burtlo/cleaner cli formatter Nov 30, 2016

burtlo commented Nov 30, 2016

View reviewed changes

arlimus reviewed Dec 14, 2016

View reviewed changes

arlimus added the in progress label Dec 14, 2016

arlimus self-assigned this Dec 14, 2016

Victoria Jeffrey and others added 3 commits December 14, 2016 13:34

wip: remove some global variable usage

37e60e9

add in some comments, etc

b30a5dc

Removes start stage from Json Formatter

a8a949d

The full JSON formatter was using the start step to setup the profiles_info. I moved that to a memozied method so that the first time it is called it will be created. Signed-off-by: Franklin Webber <franklin@chef.io>

Franklin Webber added 8 commits December 14, 2016 13:34

Updates RSpec CLI Formater to print profiles correctly

fa45c39

The profiles will display the controls with their results and then display the examples not associated with any control but within the profile. Signed-off-by: Franklin Webber <franklin@chef.io>

Fixes to address Rubocop issues and privacy

eb495e9

The class size is too big and Rubocop is right. There are a few more classes in there that could be extracted but I am going to ignore it. The other issues that it presented were fair. Signed-off-by: Franklin Webber <franklin@chef.io>

Removes profiles reader and replaces with ivar

ad82ac4

The profiles method was never public and the @profiles is clearer. Signed-off-by: Franklin Webber <franklin@chef.io>

arlimus merged commit fd76a72 into inspec:master Dec 15, 2016

arlimus removed the in progress label Dec 15, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

burtlo/cleaner cli formatter #1331

burtlo/cleaner cli formatter #1331

burtlo commented Nov 29, 2016

burtlo commented Nov 29, 2016

burtlo commented Nov 29, 2016

burtlo Nov 29, 2016

arlimus Dec 14, 2016

burtlo Nov 29, 2016 •

edited

Loading

burtlo Nov 29, 2016

chris-rock commented Nov 29, 2016

burtlo commented Nov 29, 2016

chris-rock commented Nov 29, 2016

burtlo commented Nov 30, 2016 •

edited

Loading

burtlo commented Nov 30, 2016 •

edited

Loading

chris-rock commented Nov 30, 2016

burtlo commented Nov 30, 2016 •

edited

Loading

burtlo commented Nov 30, 2016 •

edited

Loading

burtlo Nov 30, 2016

burtlo Nov 30, 2016

burtlo Nov 30, 2016

burtlo Nov 30, 2016 •

edited

Loading

burtlo commented Nov 30, 2016

burtlo commented Dec 1, 2016

burtlo commented Dec 7, 2016

burtlo commented Dec 12, 2016 •

edited

Loading

burtlo commented Dec 13, 2016 •

edited

Loading

arlimus left a comment

arlimus Dec 14, 2016

arlimus Dec 14, 2016

burtlo Dec 14, 2016

arlimus Dec 14, 2016

arlimus Dec 14, 2016

burtlo Dec 14, 2016

arlimus Dec 14, 2016

burtlo Dec 14, 2016 •

edited

Loading

burtlo commented Dec 14, 2016

arlimus commented Dec 15, 2016 •

edited

Loading

burtlo/cleaner cli formatter #1331

burtlo/cleaner cli formatter #1331

Conversation

burtlo commented Nov 29, 2016

burtlo commented Nov 29, 2016

burtlo commented Nov 29, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

burtlo Nov 29, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chris-rock commented Nov 29, 2016

burtlo commented Nov 29, 2016

chris-rock commented Nov 29, 2016

burtlo commented Nov 30, 2016 • edited Loading

burtlo commented Nov 30, 2016 • edited Loading

chris-rock commented Nov 30, 2016

burtlo commented Nov 30, 2016 • edited Loading

burtlo commented Nov 30, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

burtlo Nov 30, 2016 • edited Loading

Choose a reason for hiding this comment

burtlo commented Nov 30, 2016

burtlo commented Dec 1, 2016

burtlo commented Dec 7, 2016

burtlo commented Dec 12, 2016 • edited Loading

burtlo commented Dec 13, 2016 • edited Loading

arlimus 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

burtlo Dec 14, 2016 • edited Loading

Choose a reason for hiding this comment

burtlo commented Dec 14, 2016

arlimus commented Dec 15, 2016 • edited Loading

burtlo Nov 29, 2016 •

edited

Loading

burtlo commented Nov 30, 2016 •

edited

Loading

burtlo commented Nov 30, 2016 •

edited

Loading

burtlo commented Nov 30, 2016 •

edited

Loading

burtlo commented Nov 30, 2016 •

edited

Loading

burtlo Nov 30, 2016 •

edited

Loading

burtlo commented Dec 12, 2016 •

edited

Loading

burtlo commented Dec 13, 2016 •

edited

Loading

burtlo Dec 14, 2016 •

edited

Loading

arlimus commented Dec 15, 2016 •

edited

Loading