(SDK-147) Add 'test unit' runner and basic output formatting #98

scotje · 2017-06-21T23:58:32Z

No description provided.

DavidS

👍 on using puppetlabs_spec_helper's rake task. While I still hope we can get rid of PSH in the future, now is not the time for it, alas.

I understand this is still in raw form, so some of the comments about commit structure are probably pre-mature.

DavidS · 2017-06-22T09:33:51Z

lib/pdk/report/event.rb

+
+        valid_types = [Array]
+
+        unless valid_types.include?(value.class)


That seems a bit excessive for a single class test :->

DavidS · 2017-06-22T09:45:07Z

lib/pdk/report.rb

@@ -78,7 +78,7 @@ def to_junit(target = self.class.default_target)
    def to_text(target = self.class.default_target)
      events.each do |_tool, tool_events|
        tool_events.each do |event|
-          target.puts("\n" << event.to_text) unless event.pass?
+          target.puts(event.to_text) unless event.pass?


This just undoes a change from the previous commit. If you rebase --interactive, and remove the change from the first commit, you can get rid of it here too.

DavidS · 2017-06-22T09:46:17Z

lib/pdk/cli/exec.rb

@@ -55,6 +55,7 @@ class Command
        attr_reader :argv
        attr_reader :context
        attr_accessor :timeout
+        attr_accessor :environment


👍 on the change itself, it does feel like it could be a separate commit though, as it adds a feature unrelated to the "puppetlabs_spec_helper invocation for unit tests" mentioned in the commit message.

scotje · 2017-06-22T21:01:14Z

Yes I'll try to rebase this into a saner set of commits before merge.

scotje · 2017-06-26T23:12:13Z

@DavidS @rodjek @james-stocks This is ready to go now. Reviewing commit-by-commit should be pretty clear.

Once merged, James should be able to rebase his --list PR on top and hopefully most of the test issues will be resolved.

bmjen · 2017-06-26T23:16:47Z

lib/pdk/tests/unit.rb

+        return unless json_data['summary']
+
+        # TODO: standardize summary output
+        $stderr.puts '  ' << _('Evaluated %{total} tests in %{duration} seconds: %{errors} errors, %{failures} failures, %{pending} pending') % {


Shouldn't this summary line be added to the report to be printed out at the end?

The report may go to a file or be in XML or all kinds of things. I consider the summary to be diagnostic information which is why I write it to $stderr once the run is finished.

(Note also that a report generator could create it's own summary when appropriate based on the events we added when parsing the output.)

james-stocks · 2017-06-27T10:30:28Z

lib/pdk/tests/unit.rb

+          json_data['messages'].each do |msg|
+            report.add_event(
+              source: 'rspec',
+              state: :error,


I was looking at the rspec json output yesterday, and I was unsure if a message from rspec is necessarily an error; a quick browse of the rspec-core code suggests they're pretty neutral on what a message is.
I think it's a fair assumption that a message from rspec is something we can consider an error.

EDIT: I notice in the code below that we won't include failures in the report if ['messages'] is populated - so we assume not just that a message is an error, but additionally that a message means that the example results are going to be pointless. Are there definitely not cases (e.g. deprecation warnings) where rspec will return a message AND meaningful spec results?

EDIT: I think when there are 0 examples to run; rspec provides 'No examples found.' as a message. But once we add skeleton spec code (e.g. it compiles ) we maybe could consider this to also be an an error

james-stocks · 2017-06-27T10:33:46Z

lib/pdk/tests/unit.rb

+        elsif json_data['summary'] && json_data['summary']['failure_count'].zero?
+          report.add_event(state: :passed, severity: :ok)
+        elsif json_data['examples']
+          failures = json_data['examples'].reject { |e| e['status'] == 'passed' }


Should pending also be rejected here?

james-stocks · 2017-06-27T10:40:33Z

lib/pdk/tests/unit.rb

+            )
+          end
+        elsif json_data['summary'] && json_data['summary']['failure_count'].zero?
+          report.add_event(state: :passed, severity: :ok)


Is this all the report includes in the case of all examples passing? I guess the details of each example are not needed if everything is passing; but wouldn't it be desirable to record at least the number of examples that passed?

james-stocks

Couple of questions on the rspec reporting, otherwise LGTM

DavidS

I've reviewed a couple of examples to support or reject what @james-stocks has been talking about. Deprecations do not get taken as an error, so that's fine. They are also not displayed at all, that's not so good.

Additionally, when tests fail, important information about the failures is swallowed in the default output:

david@davids:~/git/pdk/test$ ~/git/pdk/bin/pdk test unit
[✔] Checking for missing Gemfile dependencies
[✔] Checking for required Bundler binstubs
[✖] Running unit tests
  Evaluated 3 tests in 0.28056711 seconds: 0 errors, 3 failures, 0 pending
./spec/classes/mod_spec.rb:8: failed: expected that the catalogue would not compile but it does
./spec/classes/mod_spec.rb:8: failed: expected that the catalogue would not compile but it does
./spec/classes/mod_spec.rb:8: failed: expected that the catalogue would not compile but it does
david@davids:~/git/pdk/test$

Compare this to the plain rspec output:

david@davids:~/git/pdk/test$ bundle exec rspec spec/classes/mod_spec.rb 

test::mod
  on debian-8-x86_64
    should not compile into a catalogue without dependency cycles (FAILED - 1)
  on ubuntu-16.04-x86_64
    should not compile into a catalogue without dependency cycles (FAILED - 2)
  on redhat-7-x86_64
    should not compile into a catalogue without dependency cycles (FAILED - 3)

Failures:

  1) test::mod on debian-8-x86_64 should not compile into a catalogue without dependency cycles
     Failure/Error: it { subject.should_not compile }
       expected that the catalogue would not compile but it does
     # ./spec/classes/mod_spec.rb:8:in `block (4 levels) in <top (required)>'

  2) test::mod on ubuntu-16.04-x86_64 should not compile into a catalogue without dependency cycles
     Failure/Error: it { subject.should_not compile }
       expected that the catalogue would not compile but it does
     # ./spec/classes/mod_spec.rb:8:in `block (4 levels) in <top (required)>'

  3) test::mod on redhat-7-x86_64 should not compile into a catalogue without dependency cycles
     Failure/Error: it { subject.should_not compile }
       expected that the catalogue would not compile but it does
     # ./spec/classes/mod_spec.rb:8:in `block (4 levels) in <top (required)>'

Deprecation Warnings:

Using `should_not` from rspec-expectations' old `:should` syntax without explicitly enabling the syntax is deprecated. Use the new `:expect` syntax or explicitly enable `:should` with `config.expect_with(:rspec) { |c| c.syntax = :should }` instead. Called from /home/david/git/pdk/test/spec/classes/mod_spec.rb:8:in `block (4 levels) in <top (required)>'.


If you need more of the backtrace for any of these deprecations to
identify where to make the necessary changes, you can configure
`config.raise_errors_for_deprecations!`, and it will turn the
deprecation warnings into errors, giving you the full backtrace.

1 deprecation warning total

Finished in 0.28652 seconds (files took 1.37 seconds to load)
3 examples, 3 failures

Failed examples:

rspec ./spec/classes/mod_spec.rb[1:1:1] # test::mod on debian-8-x86_64 should not compile into a catalogue without dependency cycles
rspec ./spec/classes/mod_spec.rb[1:2:1] # test::mod on ubuntu-16.04-x86_64 should not compile into a catalogue without dependency cycles
rspec ./spec/classes/mod_spec.rb[1:3:1] # test::mod on redhat-7-x86_64 should not compile into a catalogue without dependency cycles

david@davids:~/git/pdk/test$

Everything else except the actual functional change looks good. @scotje if you can post a separate PR for the first 6 commits, we can get them merged independently and reduce the size and load of this one.

scotje · 2017-06-27T18:51:14Z

The JSON output from RSpec in general leaves a lot to be desired, we might want to consider writing a our own custom reporter eventually and shipping it as part of the template.

I'll see what I can do to improve things with what we have though.

This commit implements basic support for the `test unit` subcommand which invokes the puppetlabs_spec_helper provided `rake spec` command inside the context of the target module's bundled gem environment. Output from the underlying RSpec command is parsed and then returned to the user in the requested format and to the requested destination.

scotje · 2017-06-27T21:47:08Z

Updated and simplified the report parsing.

Currently some of the information (such as context) is being passed to the reporter but the to_text method of the Report::Event class could use improvement. That seems like a separate PR unless there is something this code isn't passing to the reporter that it should be.

james-stocks · 2017-06-28T08:34:00Z

lib/pdk/tests/unit.rb

+        example_results.each do |result, examples|
+          state = case result
+                  when 'passed' then :passed
+                  when 'failed' then :failure


It looks like at this point we immediately translate rspec result values to the result values our CI uses

I think our report code ( https://github.com/puppetlabs/pdk/blob/master/lib/pdk/report/event.rb#L83 ) should be clearly documented to explain that rspec pending gets mapped to a skipped result in XML reports; I'll update that outside of this PR

james-stocks · 2017-06-28T10:17:31Z

lib/pdk/tests/unit.rb

+
+        example_results.each do |result, examples|
+          # Translate rspec example results to JUnit XML testcase results
+          state = case result


FYI on seeing this, I was a little unsure about the translation of rspec result values to CI result values. I added a comment line here; and also created #115 (now merged) to support pending/skipped results better.
Now LGTM 👍

DavidS reviewed Jun 22, 2017

View reviewed changes

scotje force-pushed the 147_unit_test_runner branch 2 times, most recently from 282a78b to 6cc293c Compare June 22, 2017 19:27

scotje force-pushed the 147_unit_test_runner branch 3 times, most recently from 93b11a4 to ead3e3b Compare June 26, 2017 21:00

bmjen reviewed Jun 26, 2017

View reviewed changes

james-stocks added the feature label Jun 27, 2017

james-stocks reviewed Jun 27, 2017

View reviewed changes

DavidS suggested changes Jun 27, 2017

View reviewed changes

This was referenced Jun 27, 2017

(MAINT) Various CLI::Exec improvements and updates #111

Merged

(MAINT) Add support for stacktrace to Report::Event class #112

Merged

scotje force-pushed the 147_unit_test_runner branch from ead3e3b to dd03a74 Compare June 27, 2017 21:37

james-stocks reviewed Jun 28, 2017

View reviewed changes

(maint) Add comment to unit test reporting code

3699983

james-stocks reviewed Jun 28, 2017

View reviewed changes

DavidS merged commit 8161e94 into puppetlabs:master Jun 28, 2017

DavidS mentioned this pull request Jun 28, 2017

(SDK-147) Improve text report formatting #116

Closed

scotje deleted the 147_unit_test_runner branch June 28, 2017 17:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(SDK-147) Add 'test unit' runner and basic output formatting #98

(SDK-147) Add 'test unit' runner and basic output formatting #98

scotje commented Jun 21, 2017 •

edited

Loading

DavidS left a comment

DavidS Jun 22, 2017

DavidS Jun 22, 2017

DavidS Jun 22, 2017

scotje commented Jun 22, 2017

scotje commented Jun 26, 2017

bmjen Jun 26, 2017

scotje Jun 26, 2017

scotje Jun 26, 2017

james-stocks Jun 27, 2017 •

edited

Loading

james-stocks Jun 27, 2017

james-stocks Jun 27, 2017

james-stocks left a comment

DavidS left a comment

scotje commented Jun 27, 2017

scotje commented Jun 27, 2017

james-stocks Jun 28, 2017

james-stocks Jun 28, 2017 •

edited

Loading


		valid_types = [Array]

		unless valid_types.include?(value.class)

(SDK-147) Add 'test unit' runner and basic output formatting #98

(SDK-147) Add 'test unit' runner and basic output formatting #98

Conversation

scotje commented Jun 21, 2017 • edited Loading

DavidS 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

scotje commented Jun 22, 2017

scotje commented Jun 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

james-stocks Jun 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

james-stocks left a comment

Choose a reason for hiding this comment

DavidS left a comment

Choose a reason for hiding this comment

scotje commented Jun 27, 2017

scotje commented Jun 27, 2017

Choose a reason for hiding this comment

james-stocks Jun 28, 2017 • edited Loading

Choose a reason for hiding this comment

scotje commented Jun 21, 2017 •

edited

Loading

james-stocks Jun 27, 2017 •

edited

Loading

james-stocks Jun 28, 2017 •

edited

Loading