Various changes to get specs to pass on windows #225

jaym · 2014-11-11T04:13:29Z

Fixed up cli_specs (there were some system dependent function calls that needed to be stubbed)
Cookbook profiler git_specs needed some modifications to get them to work with cmd.exe (' -> ")
Policy store_config_specs used File.expand_path, which is system dependent
Comparing their SHAs was broken because windows decides to do magic unless you give it the b mode when writing files

tyler-ball · 2014-11-11T20:24:19Z

lib/chef-dk/policyfile_services/install.rb

@@ -93,7 +93,7 @@ def generate_lock_and_install

        lock_data = policyfile_compiler.lock.to_lock

-        File.open(policyfile_lock_expanded_path, "w+") do |f|
+        File.open(policyfile_lock_expanded_path, "wb+") do |f|


This changes the encoding to ASCII-8BIT doesn't it?

I'm not 100% sure about it becoming ASCII.
I do know that on windows, it will do a bunch of things if you don't specify binary mode.
In text mode, carriage return–linefeed combinations are translated into single linefeeds on input, and linefeed characters are translated to carriage return–linefeed combinations on output. When a Unicode stream-I/O function operates in text mode (the default), the source or destination stream is assumed to be a sequence of multibyte characters. Therefore, the Unicode stream-input functions convert multibyte characters to wide characters (as if by a call to the mbtowc function). For the same reason, the Unicode stream-output functions convert wide characters to multibyte characters (as if by a call to the wctomb function).

http://www.ruby-doc.org/core-2.1.4/IO.html#method-c-new

Check out the "b" flag description there. We may already be specifying UTF8 encoding somewhere?

I'm not sure that this will matter for writing. I'll try some things out.

tyler-ball · 2014-11-11T20:36:00Z

LGTM

danielsdeleo · 2014-11-11T21:18:20Z

spec/unit/cookbook_profiler/git_spec.rb

@@ -93,7 +93,7 @@ def edit_repo
        edit_repo
        system_command('git config --local user.name "Alice"', cwd: cookbook_path).error!
        system_command('git config --local user.email "alice@example.com"', cwd: cookbook_path).error!
-        system_command("git commit -a -m 'update readme' --author 'Alice <alice@example.com>'", cwd: cookbook_path).error!
+          system_command('git commit -a -m "update readme" --author "Alice <alice@example.com>"', cwd: cookbook_path).error!


What's this all about? Also, looks like this adds two extra spaces?

Spaces: I had to create a fresh vm to develop on. And vim with default
settings is stupid.
Quotes: cmd.exe is stupid. It freaks out when using single quotes

On Tue, Nov 11, 2014 at 1:18 PM, Dan DeLeo notifications@github.com wrote:

In spec/unit/cookbook_profiler/git_spec.rb:

@@ -93,7 +93,7 @@ def edit_repo
edit_repo
system_command('git config --local user.name "Alice"', cwd: cookbook_path).error!
system_command('git config --local user.email "alice@example.com"', cwd: cookbook_path).error!

system_command("git commit -a -m 'update readme' --author 'Alice alice@example.com'", cwd: cookbook_path).error!

system_command('git commit -a -m "update readme" --author "Alice alice@example.com"', cwd: cookbook_path).error!

What's this all about? Also, looks like this adds two extra spaces?

—
Reply to this email directly or view it on GitHub
/~https://github.com/opscode/chef-dk/pull/225/files#r20181106.

danielsdeleo · 2014-11-11T21:48:55Z

Fix the spaces and then

sersut · 2014-11-12T20:22:41Z

lib/chef-dk/policyfile_services/push.rb

@@ -89,7 +89,7 @@ def upload_policy
      end

      def write_updated_lockfile
-        File.open(policyfile_lock_expanded_path, "w+") do |f|
+        File.open(policyfile_lock_expanded_path, "wb+") do |f|


If this is what we need, we can have a helper with_file and make sure we don't break when we call File.open() somewhere else later on.

jaym · 2014-11-12T21:13:58Z

@sersut factored out the File.open stuff

bwmeier · 2014-11-12T21:55:29Z

FYI - there's an impact of #180 on this fix - the scope of this should be limited to cli_spec.rb. The code change I made is below, but this did not include the fixes in this PR.

diff --git a/spec/unit/cli_spec.rb b/spec/unit/cli_spec.rb
index 8144602..b91ebb5 100644
--- a/spec/unit/cli_spec.rb
+++ b/spec/unit/cli_spec.rb
@@ -167,6 +167,12 @@ E

   context "sanity_check!" do
     context "on unix" do
+      before do
+        allow(Chef::Platform).to receive(:windows?).and_return(false)
+        stub_const("File::PATH_SEPARATOR", ':')
+        allow(cli).to receive(:path_key).and_return('PATH')
+      end
+
       it "complains if embedded is first" do
         expect(cli).to receive(:env).and_return({'PATH' => '/opt/chefdk/embedded/bin:/opt/chefdk/bin' })
         allow(cli).to receive(:omnibus_embedded_bin_dir).and_return("/opt/chefdk/embedded/bin")
@@ -208,6 +214,7 @@ E
       before do
         allow(Chef::Platform).to receive(:windows?).and_return(true)
         stub_const("File::PATH_SEPARATOR", ';')
+        allow(cli).to receive(:path_key).and_return('PATH')
       end

       it "complains if embedded is first" do

jaym · 2014-11-12T22:02:30Z

@bwmeier I did something slightly different, it should have the same end result.

bwmeier · 2014-11-12T22:38:19Z

Oh, after applying the #180 fix, I also had to include a change to exec_spec.rb to explicitly set the File::PATH_SEPARATOR:

diff --git a/spec/unit/command/exec_spec.rb b/spec/unit/command/exec_spec.rb
index ba14724..6af639d 100644
--- a/spec/unit/command/exec_spec.rb
+++ b/spec/unit/command/exec_spec.rb
@@ -80,6 +80,7 @@ describe ChefDK::Command::Exec do
       before do
         allow(command_instance).to receive(:omnibus_embedded_bin_dir).and_return(omnibus_embedded_bin_dir)
         allow(command_instance).to receive(:omnibus_bin_dir).and_return(omnibus_bin_dir)
+        stub_const("File::PATH_SEPARATOR", ':')
       end

       it "should call exec to fire off the command with the correct environment" do
@@ -115,6 +116,7 @@ describe ChefDK::Command::Exec do
       before do
         allow(command_instance).to receive(:omnibus_embedded_bin_dir).and_return(omnibus_embedded_bin_dir)
         allow(command_instance).to receive(:omnibus_bin_dir).and_return(omnibus_bin_dir)
+        stub_const("File::PATH_SEPARATOR", ':')
       end

       it "should raise Errno::ENOENT" do

jaym · 2014-11-12T23:35:01Z

That makes sense.

sersut · 2014-11-12T23:36:53Z

lib/chef-dk/helpers.rb

@@ -103,5 +103,8 @@ def omnibus_env
        end
    end

+    def with_file(path, mode='wb+', &block)


Let's add some comments here for why choosing this mode by default.

sersut · 2014-11-12T23:38:15Z

Overall 👍

Various changes to get specs to pass on windows

Chef DK Release Prep

chef-supermarket added the Signed CLA label Nov 11, 2014

tyler-ball reviewed Nov 11, 2014
View reviewed changes

danielsdeleo reviewed Nov 11, 2014
View reviewed changes

jaym added 4 commits November 11, 2014 14:40

cli_specs pass on Windows

3615d7d

Cookbook profiler git_specs pass on windows.

3c291a2

policyfile store_config_spec passes on windows

cdd506b

Replaced w+ with wb+ to make comparisons on windows happy

24690bf

jaym force-pushed the jdmundrawala/unit-tests-windows branch from 1afa88f to 24690bf Compare November 11, 2014 23:01

chef-supermarket added the Signed CLA label Nov 11, 2014

sersut reviewed Nov 12, 2014
View reviewed changes

Factor out File.open(path, "wb+", block) into method

d27ffdf

danielsdeleo mentioned this pull request Nov 12, 2014

Patch to chef-dk to resolve issue #186 #187

Closed

bwmeier mentioned this pull request Nov 12, 2014

Windows PATH separator wrong in Chef exec command #180

Closed

sersut reviewed Nov 12, 2014
View reviewed changes

Added comment for with_file

5e6188f

chef-supermarket added the Signed CLA label Nov 12, 2014

jaym added a commit that referenced this pull request Nov 13, 2014

Merge pull request #225 from opscode/jdmundrawala/unit-tests-windows

f3dd803

Various changes to get specs to pass on windows

jaym merged commit f3dd803 into master Nov 13, 2014

jaym deleted the jdmundrawala/unit-tests-windows branch November 13, 2014 04:53

ksubrama pushed a commit that referenced this pull request Jan 11, 2016

Merge pull request #225 from opscode/sersut/dk-rel-prep

25c19f6

Chef DK Release Prep

chef-boneyard locked and limited conversation to collaborators Feb 14, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Various changes to get specs to pass on windows #225

Various changes to get specs to pass on windows #225

jaym commented Nov 11, 2014

tyler-ball Nov 11, 2014

jaym Nov 11, 2014

tyler-ball Nov 11, 2014

jaym Nov 11, 2014

tyler-ball commented Nov 11, 2014

danielsdeleo Nov 11, 2014

jaym Nov 11, 2014

danielsdeleo commented Nov 11, 2014

sersut Nov 12, 2014

jaym commented Nov 12, 2014

bwmeier commented Nov 12, 2014

jaym commented Nov 12, 2014

bwmeier commented Nov 12, 2014

jaym commented Nov 12, 2014

sersut Nov 12, 2014

sersut commented Nov 12, 2014

Various changes to get specs to pass on windows #225

Various changes to get specs to pass on windows #225

Conversation

jaym commented Nov 11, 2014

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

tyler-ball commented Nov 11, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielsdeleo commented Nov 11, 2014

Choose a reason for hiding this comment

jaym commented Nov 12, 2014

bwmeier commented Nov 12, 2014

jaym commented Nov 12, 2014

bwmeier commented Nov 12, 2014

jaym commented Nov 12, 2014

Choose a reason for hiding this comment

sersut commented Nov 12, 2014