Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make sure the path is correctly escaped in case it contains spaces. #1045

Merged
merged 5 commits into from
Mar 31, 2018

Conversation

andrewmarkle
Copy link
Contributor

@andrewmarkle andrewmarkle commented Mar 11, 2018

Closes #1044. This fixes an issue where the test runner fails if you have spaces in your path.

This PR uses Regexp to escape the path before the command is run.


This change is Reviewable

@@ -146,7 +146,7 @@ def required(arg_name)
end

def self.prepend_cd_node_modules_directory(cmd)
"cd #{ReactOnRails.configuration.node_modules_location} && #{cmd}"
"cd #{Regexp::escape(ReactOnRails.configuration.node_modules_location.to_s)} && #{cmd}"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this the right solution for shell escaping?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if put quotes around the value:

"cd \"#{ReactOnRails.configuration.node_modules_location}\" && #{cmd}"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea! Didn't think of that. I like that solution better!

@justin808
Copy link
Member

Travis failure:

Offenses:
lib/react_on_rails/utils.rb:149:19: C: Style/ColonMethodCall: Do not use :: for method calls.
      "cd #{Regexp::escape(ReactOnRails.configuration.node_modules_location.to_s)} && #{cmd}"
                  ^^
139 files inspected, 1 offense detected
rake aborted!

@justin808
Copy link
Member

@andrewmarkle we have this CI failure:

Failures:
  1) ReactOnRails::TestHelper::WebpackAssetsCompiler#ensureAssetsCompiled when assets compiler command is invalid prints the correct message
     Failure/Error:
       expect do
         begin
           ReactOnRails::TestHelper::WebpackAssetsCompiler.new.compile_assets
           # rubocop:disable Lint/HandleExceptions
         rescue SystemExit
           # No op
         end
         # rubocop:enable Lint/HandleExceptions
       end.to output(/#{expected_output}/).to_stdout
     
       expected block to output /React on Rails FATAL ERROR!
       Error in building webpack assets!
       cmd: cd  && sh -c 'exit 1'
       / to stdout, but output "\nBuilding Webpack assets...\n\e[31m================================================================...tatus: 1\n================================================================================\n\e[0m\n"
       Diff:
       
       @@ -1,5 +1,10 @@
       -/React on Rails FATAL ERROR!
       +
       +Building Webpack assets...
       +================================================================================
       +React on Rails FATAL ERROR!
        Error in building webpack assets!
       -cmd: cd  && sh -c 'exit 1'
       -/
       +cmd: cd "" && sh -c 'exit 1'
       +exitstatus: 1
       +================================================================================
       +
       
     # ./spec/react_on_rails/test_helper/webpack_assets_compiler_spec.rb:30:in `block (4 levels) in <top (required)>'
     # /home/travis/.rvm/gems/ruby-2.5.0/gems/rspec-retry-0.5.5/lib/rspec/retry.rb:115:in `block in run'
     # /home/travis/.rvm/gems/ruby-2.5.0/gems/rspec-retry-0.5.5/lib/rspec/retry.rb:104:in `loop'
     # /home/travis/.rvm/gems/ruby-2.5.0/gems/rspec-retry-0.5.5/lib/rspec/retry.rb:104:in `run'
     # /home/travis/.rvm/gems/ruby-2.5.0/gems/rspec-retry-0.5.5/lib/rspec_ext/rspec_ext.rb:12:in `run_with_retry'
     # /home/travis/.rvm/gems/ruby-2.5.0/gems/rspec-retry-0.5.5/lib/rspec/retry.rb:33:in `block (2 levels) in setup'
Finished in 1 minute 18.36 seconds (files took 1.29 seconds to load)
144 examples, 1 failure
Failed examples:
rspec ./spec/react_on_rails/test_helper/webpack_assets_compiler_spec.rb:23 # ReactOnRails::TestHelper::WebpackAssetsCompiler#ensureAssetsCompiled when assets compiler command is invalid prints the correct message

@coveralls
Copy link

Coverage Status

Coverage remained the same at ?% when pulling 144ecba on andrewmarkle:escape-path-fix into a6fc8d8 on shakacode:master.

@coveralls
Copy link

coveralls commented Mar 23, 2018

Coverage Status

Coverage remained the same at ?% when pulling 455b33e on andrewmarkle:escape-path-fix into 6b3641a on shakacode:master.

@andrewmarkle
Copy link
Contributor Author

Thanks @justin808 for your patience! Should be good to go now.

@justin808
Copy link
Member

Reviewed 1 of 1 files at r2, 1 of 1 files at r3.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


spec/react_on_rails/test_helper/webpack_assets_compiler_spec.rb, line 27 at r3 (raw file):

          React on Rails FATAL ERROR!
          Error in building webpack assets!
          cmd: cd \"#{Rails.root}\" && #{invalid_command}

If this is in a heredoc, why do you escape the "? @andrewmarkle


Comments from Reviewable

@justin808
Copy link
Member

Please add a changelog entry


Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


Comments from Reviewable

@justin808
Copy link
Member

@andrewmarkle ?

@andrewmarkle
Copy link
Contributor Author

andrewmarkle commented Mar 31, 2018

If this is in a heredoc, why do you escape the "?

I couldn't get this to work otherwise. If you just add in the escape characters, the heredoc ignores the interpolation and it outputs #{Rails.root} as part of the string.

Do you have another suggestion?

@justin808
Copy link
Member

LGTM


Reviewed 1 of 1 files at r4.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


Comments from Reviewable

@justin808 justin808 merged commit a1932d9 into shakacode:master Mar 31, 2018
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.

Can't run the specs from a path with spaces
3 participants