Batch commands

[blairconrad]

Fixes #207.

@haf, I have to ask forgiveness. I have broken the cardinal rule of Pull Requests by substantially changing the base code, and its style. I fear it will not be to your taste, and apologize. But if you have patience to work with me, I'm happy to fix anything you don't like.

An explanation:

In order to test the test-batching, I found that I would need access to the TestRunner::Cmd objects that the TestRunner::Task was making. Relying on creating a Cmd in the specs and investigating its behaviour would no longer work. So I made a change to the Task to support this. At the same time, it became clear that poking execute_tests_for dll wouldn't work either. So I, in stages, replaced most of the specs so they would be driven from the Task level, and cover (nearly) all behaviour in the Task and Cmds. It's counter to the existing approach, but is more refactoring-friendly, and is an approach that I've seen work very well in other projects. By driving the code in a way that more closely matches how the users would, you can worry a little less about the internal details.

So:

1. I introduced a private execute_commands method on the Task, which is overridden in the specs. The original behaviour is so simple that not exercising it during the specs is hopefully forgivable. I'm not usually a fan of overriding production methods in tests, but it seems no worse than sending a signal to a private method.
2. I updated existing specs to provide close to end-to-end coverage, from the config object down to the command, to mirror how the new specs would be written.
3. I added minor extra specs, e.g. for is_ms_test. I did not add an end-to-end spec for copy_local, as I wasn't sure you'd want to add all that extra copying around during test runs
4. The new specs change their working directory to better work with existing "test" files. The task requires test files to actually exist, so I co-opted existing files in the hierarchy. By running in the specs directory, it's possible to test a negative relative path exactly. I know, perhaps this was an odd choice.
5. Finally introduced specs for the execute_as_batch option, and implemented the feature. There's no "path shortening" feature for this yet - the tests would execute in the current directory. If you'd like, I can see about looking for the lowest common directory above the "test files" and execute from there, or some such. Or this could be introduced later, if you think it worthwhile.

So, yeah. Perhaps not what you were expecting. And maybe totally against the style you'd like to have for the tests. And my lack of ruby/rspec knowledge probably means I've made a terrible mess of all of it. Sorry. But again, if you're willing, I'll work to mold things into a shape you prefer.

Thanks for considering the PR.

[haf]

Hi @blairconrad,

Contrary to your beliefs, I do enjoy seeing PRs that change structure as long as they are justified. In your PR, I think it's exactly that. I can't fault your justification and you've written the tests. Your ruby is nice and easy to read. What's not to like?

I re-ran the build and it passes. So if you'd answer my nit-picking comments in the code, we'd be close to merging 👍

Cheers, Henrik

[blairconrad]

Contrary to your beliefs, I do enjoy seeing PRs that change structure as long as…

It wasn't a specific believe about you or anything. I just know it's often a shock to open up a PR and see that a potential contributor has changed the world. And sometimes for no other reason than that "they like it better that way". I though I had good reasons for most of my changes, but from inside my brain, how could I be sure?

Thanks for the comments on the ruby. I scoured the existing code and the docs to try to find what I thought would be correct and somewhat idiomatic. Aside from the FakeItEasy rakefile, this is the only ruby I've ever written. So I assumed I'd mess it up.

I appreciate the quick review. I will get started on the changes. Do you have a preference between me amending my previous commits or adding additional commit(s) to address your concerns? I've noticed some people really like one or the other.

[haf]

Do you have a preference between me amending my previous commits or adding additional commit(s) to address your concerns?

Either way is fine.

[blairconrad]

Either way is fine.

Thanks, @haf. I amended the commits, since it made them tidier, I think. Two commits have disappeared. One where I removed some config tests, and one where I had switched to executing the subject task as soon as it was created.

In case it makes it easier, here's the diff from the original PR state to the new one:

git diff 2ba65ce06c4f63b68661354041a175715e2858ce f8ea3b0cde789964b0ce5e74d4ecd605f2151f31

diff --git a/lib/albacore/task_types/test_runner.rb b/lib/albacore/task_types/test_runner.rb
index efe4a69..f12114b 100644
--- a/lib/albacore/task_types/test_runner.rb
+++ b/lib/albacore/task_types/test_runner.rb
@@ -87,7 +87,12 @@ module Albacore
       # work_dir parameter
       def initialize work_dir, executable, parameters, files, clr_command = true
         @work_dir, @executable = work_dir, executable
-        @parameters = files.concat(parameters.to_a)
+        if files.respond_to? :each
+          @parameters = files.to_a.concat(parameters.to_a)
+        else
+          @parameters = parameters.to_a.unshift(files)
+        end
+
         @clr_command = clr_command
       end

diff --git a/spec/test_runner_spec.rb b/spec/test_runner_spec.rb
index 2269e4a..0b85f7d 100644
--- a/spec/test_runner_spec.rb
+++ b/spec/test_runner_spec.rb
@@ -4,11 +4,86 @@ require 'map'

 describe ::Albacore::TestRunner::Config do
   it do
+    should respond_to :opts
+  end
+  it do
+    should respond_to :files=
+  end
+  it do
     should_not respond_to :files
   end
   it do
     should respond_to :copy_local
   end
+  it do
+    should respond_to :exe=
+  end
+  it do
+    should respond_to :native_exe
+  end
+  it do
+    should respond_to :execute_as_batch
+  end
+end
+
+describe ::Albacore::TestRunner::Config do
+  subject do
+    ::Albacore::TestRunner::Config.new
+  end
+
+  before :each do
+    subject.add_parameter '/TestResults=/b/c/d/e.xml'
+    subject.native_exe
+  end
+
+  it 'should have the appropriate parameter in #opts.get(:parameters)' do
+    expect(subject.opts.get(:parameters)).to include('/TestResults=/b/c/d/e.xml')
+  end
+
+  it 'should have clr_command=false' do
+    expect(subject.opts.get(:clr_command)).to be false
+  end
+end
+
+describe 'the order of which parameters are passed', ::Albacore::TestRunner::Config do
+  subject do
+    config = ::Albacore::TestRunner::Config.new
+    config.files = 'a/b/c/file.dll'
+    config.exe   = 'test-runner.exe'
+    config.add_parameter '/TestResults=abc.xml'
+    config
+  end
+
+  let :params do
+    subject.opts.get(:parameters)
+  end
+
+  it 'should first pass the flags' do
+    expect(params.first).to eq('/TestResults=abc.xml')
+  end
+
+  it 'should pass the file as a :files' do
+    expect(subject.opts.get(:files)).to eq(['a/b/c/file.dll'])
+  end
+end
+
+describe ::Albacore::TestRunner::Cmd do
+  subject do
+    cmd = ::Albacore::TestRunner::Cmd.new 'work_dir', 'run-tests.exe', %w[params go here], 'a/b/c/lib.tests.dll'
+    cmd.extend ShInterceptor
+    cmd.execute
+    cmd
+  end
+
+  it 'should include the parameters when executing' do
+    # the intersection of actual parameters with expected should eq expected
+    expect(subject.parameters - (subject.parameters - %w|params go here|)).
+      to eq(%w|params go here|)
+  end
+
+  it 'should give the full path when executing' do
+    expect((subject.parameters - %w|params go here|)).to eq(%w|a/b/c/lib.tests.dll|)
+  end
 end

 describe ::Albacore::TestRunner::Task, 'when configured improperly' do
@@ -229,3 +304,37 @@ describe ::Albacore::TestRunner::Task do
     end
   end
 end
+
+describe ::Albacore::TestRunner::Cmd do
+  describe "creation" do
+    subject do
+      command = ::Albacore::TestRunner::Cmd.new '.',
+                                                'test-runner.exe',
+                                                ['/parameter'],
+                                                files
+      command.extend ShInterceptor
+      command.execute
+      command
+    end
+
+    context "files is a single file" do
+      let :files do
+        'file'
+      end
+
+      it "should prepend the file to the parameters list" do
+        expect(subject.invocations[0].parameters).to eq(['file', '/parameter'])
+      end
+    end
+
+    context "files is an array" do
+      let :files do
+        ['file1', 'file2']
+      end
+
+      it "should prepend the files to the parameters list" do
+        expect(subject.invocations[0].parameters).to eq(['file1', 'file2', '/parameter'])
+      end
+    end
+  end
+end

[blairconrad]

Oh, and I haven't forgotten about the wiki, but figure you'd want that updated at or near release time, no?

[haf]

Hi there, feel free to update the wiki now.

[haf]

Thank you for your help Blair! v.2.6.0 released

[blairconrad]

@haf, the wiki has been updated.

Thanks for having me. I had a really fun time!