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

Fix tests on Windows #353

Merged
merged 17 commits into from Nov 24, 2013

Conversation

Projects
None yet
2 participants
@raphinesse
Copy link
Contributor

commented Nov 21, 2013

  • Skip problematic gems on Windows
  • Skip tests for features unavailable in Windows
  • Fix checks for external commands to work in Windows
  • etc.

This changes test statistics on my Windows 7 machine from
612 tests, 1602 assertions, 2 failures, 18 errors, 12 skips to 612 tests, 1601 assertions, 1 failures, 2 errors, 29 skips

@@ -140,6 +138,7 @@ def test_pygmentize
end

def test_pygmentsrb
skip "pygments.rb does not support Windows" if Bundler::WINDOWS

This comment has been minimized.

Copy link
@ddfreyne

ddfreyne Nov 21, 2013

Member

This will only work if Bundler is present and used. I’d rather check RUBY_PLATFORM directly, i.e.

skip "pygments.rb does not support Windows" if RUBY_PLATFORM =~ /mswin|mingw/

This comment has been minimized.

Copy link
@raphinesse

raphinesse Nov 21, 2013

Author Contributor

Yeah, didn't know if I could assume developers working with bundler. Had the other version before but thought it would be nice to have a single expression handle the detection. Do you have any objections to a constant that's defined somewhere in helper.rb? BTW: bundler's check is broader

This comment has been minimized.

Copy link
@ddfreyne

ddfreyne Nov 21, 2013

Member

Agreed that a check like this should go into a function. I think #on_windows? in Nanoc::TestHelpers would be a good place to put this. Another benefit of putting it in a function is that you can memoize it.

I personally don’t really care about testing for msdos or djgpp (nobody uses these anyway).

This comment has been minimized.

Copy link
@raphinesse

raphinesse Nov 21, 2013

Author Contributor

I don't care about the other compilers either, I use mingw 😉

I initially wanted to go for a constant because the OS isn't very likely to change during test execution. But I'm in for a method just as well, your call.

This comment has been minimized.

Copy link
@ddfreyne

ddfreyne Nov 21, 2013

Member

I prefer a (memoized) method.

This comment has been minimized.

Copy link
@raphinesse

raphinesse Nov 21, 2013

Author Contributor

Alright, where would I memoize it? Is there some lib method available?

This comment has been minimized.

Copy link
@ddfreyne

ddfreyne Nov 22, 2013

Member

Hmm, good question. It’s not quite easy to find a place to memoize it once.

On second thought, a constant would also be fine. :)

if `which asciidoc`.strip.empty?
skip "could not find asciidoc"
end
skip_unless_have_command "asciidoc"

This comment has been minimized.

Copy link
@ddfreyne

ddfreyne Nov 21, 2013

Member

👍 for introducing #skip_unless_have_command

@@ -213,6 +213,25 @@ def with_env_vars(hash, &block)
orig_env_hash.each_pair { |k,v| ENV[k] = v }
end

def skip_unless_have_command(cmd)
unavailable = if Bundler::WINDOWS

This comment has been minimized.

Copy link
@ddfreyne

ddfreyne Nov 21, 2013

Member

Bundler::WINDOWS again (same as above)

end

def skip_unless_have_symlink
File.symlink nil, nil

This comment has been minimized.

Copy link
@ddfreyne

ddfreyne Nov 21, 2013

Member

Oh, it is unfortunate that Ruby on Windows does not support symlinks (even though Windows itself does support it).

Could you change this line to a proper test with actual files? I don’t trust nil/nil being appropriate enough.

This comment has been minimized.

Copy link
@raphinesse

raphinesse Nov 21, 2013

Author Contributor

I thought the same about not supporting a feature that's actually there

About the trust thing:
https://github.com/ruby/ruby/blob/trunk/test/fileutils/test_fileutils.rb#L51

This comment has been minimized.

Copy link
@ddfreyne

ddfreyne Nov 21, 2013

Member

OK, I can live with that.

def skip_unless_have_symlink
File.symlink nil, nil
rescue NotImplementedError => e
skip e.message

This comment has been minimized.

Copy link
@ddfreyne

ddfreyne Nov 21, 2013

Member

I’d rather have a clearer message:

skip "Symlinks are not supported by Ruby on Windows."

This comment has been minimized.

Copy link
@raphinesse

raphinesse Nov 21, 2013

Author Contributor

The message provided is actually pretty clear
symlink() function is unimplemented on this machine

I can still change it if you want though...

This comment has been minimized.

Copy link
@ddfreyne

ddfreyne Nov 21, 2013

Member

I’d change it.

@@ -3,14 +3,14 @@ source "http://rubygems.org"
gemspec

gem 'adsf'
gem 'bluecloth'
gem 'bluecloth', :platforms => :ruby

This comment has been minimized.

Copy link
@ddfreyne

ddfreyne Nov 21, 2013

Member

Out of curiosity, why does this (and handlebars) need :platforms => :ruby?

This comment has been minimized.

Copy link
@raphinesse

raphinesse Nov 21, 2013

Author Contributor

There's a pretty thorough explanation in the commit message. Long story short: both gems won't compile out of the box on windows, libv8 which is a transitive dep for handlebars is neigh impossibru

This comment has been minimized.

Copy link
@ddfreyne

ddfreyne Nov 21, 2013

Member

Answering my own question here: see Gemfile documentation.

@@ -8,6 +8,7 @@ GEM
remote: http://rubygems.org/
specs:
RedCloth (4.2.9)
RedCloth (4.2.9-x86-mingw32)

This comment has been minimized.

Copy link
@ddfreyne

ddfreyne Nov 21, 2013

Member

I’d rather leave Gemfile.lock intact. The next time I check in Gemfile.lock, all the -mingw32 stuff will be gone anyway (and presumably this would break a bundle install on non-Windows machines).

This comment has been minimized.

Copy link
@raphinesse

raphinesse Nov 21, 2013

Author Contributor

Alright, wasn't sure about that either...

@ddfreyne

This comment has been minimized.

Copy link
Member

commented Nov 21, 2013

What are the remaining errors and failures now?

@ddfreyne

This comment has been minimized.

Copy link
Member

commented Nov 21, 2013

By the way, the changes you introduced work fine on my Mac OS X box too.

@raphinesse

This comment has been minimized.

Copy link
Contributor Author

commented Nov 21, 2013

Glad to hear that

Remaining errors and failures. Curiosly the second one comes and goes. don't know why.

@ddfreyne

This comment has been minimized.

Copy link
Member

commented Nov 21, 2013

OK, would be cool if you can fix those, but otherwise I’ll try to set up Ruby on my Windows machine and give it a shot.

👍 apart from the remarks!

@raphinesse

This comment has been minimized.

Copy link
Contributor Author

commented Nov 21, 2013

OK, will change the PR according to your remarks. Not sure about the three remaining errors/failures though. Any thoughts on that?

BTW, can I display the skipped tests too?

`where #{cmd} 2> NUL`
!$?.success?
else
`which #{cmd}`.strip.empty?

This comment has been minimized.

Copy link
@raphinesse

raphinesse Nov 21, 2013

Author Contributor

which manpage states that "Which returns the number of failed arguments", so !system("which #{cmd}") should work just as well. I would use system for the check on windows too and could drop both !, changing unavailable to available. Less negation. Objections?

This comment has been minimized.

Copy link
@ddfreyne
@ddfreyne

This comment has been minimized.

Copy link
Member

commented Nov 22, 2013

Printing skipped tests by default: yes please!

@raphinesse

This comment has been minimized.

Copy link
Contributor Author

commented Nov 22, 2013

There does not seem to be a way to print skipped tests without going full blown verbose. MiniTest docs are a bad joke, btw 😡

Maybe I get to take a look at the 2-3 remaining errors today.

Actually all three newly introduced boolean methods would be candidates for memoization, but as you said, it's tricky to find a place as it is. I'd vote for just ignoring the probably marginal performance impact 😁

@raphinesse

This comment has been minimized.

Copy link
Contributor Author

commented Nov 22, 2013

OK, I got the bug causing randomly failing rendering helper tests. Two tests didn't close the file handles on their layout which caused the next test to fail.

@ddfreyne

This comment has been minimized.

Copy link
Member

commented Nov 22, 2013

This is great. If there’s nothing else you want to modify, I’ll gladly merge this.

@raphinesse

This comment has been minimized.

Copy link
Contributor Author

commented Nov 22, 2013

I'll see if I can purge the last failure. Reading empty front matter from UTF-8 with BOM. If I can't fix it, this is it.

@raphinesse

This comment has been minimized.

Copy link
Contributor Author

commented Nov 22, 2013

I could probably fix the bug by using r:bom|utf-8 as read mode here. Shouldn't interfere with other explicitly set encodings either. But is available only since 1.9.2. I guess 1.8 is still supported by nanoc?

@ddfreyne

This comment has been minimized.

Copy link
Member

commented Nov 22, 2013

Hmm, the UTF-8 issue is annoying. r:bom|utf-8 is not an option because nanoc 3.x is compatible with 1.8.7 and up (but will drop 1.8.x support in nanoc 4.0).

@raphinesse

This comment has been minimized.

Copy link
Contributor Author

commented Nov 22, 2013

OK, could you tell me which edge case is covered by this if statement? I might have a backwards compatible solution.

@ddfreyne

This comment has been minimized.

Copy link
Member

commented Nov 22, 2013

That line evaluates to true on Ruby 1.9.x, but to false on Ruby 1.8.x.

@raphinesse

This comment has been minimized.

Copy link
Contributor Author

commented Nov 22, 2013

OK, it just came to me that the actual issue is the test. My fix would allow to correctly strip the BOM from files which have been interpreted with a wrong encoding (which happens when you use win and utf-8 without setting @config[:encoding]). But that's pretty much useless unless you don't use any special chars in your document.

So the solution would probably be that the mentioned test ensures that @config[:encoding] is set to utf-8.

@raphinesse

This comment has been minimized.

Copy link
Contributor Author

commented Nov 22, 2013

Here we go, 612 tests, 1605 assertions, 0 failures, 0 errors, 29 skips

@ddfreyne

This comment has been minimized.

Copy link
Member

commented Nov 23, 2013

Looking good. One minor change is necessary for non-Windows platforms though:

system(on_windows? ? "where #{cmd} 2> NUL" : "which #{cmd}")

This prints the command when found, which is not great. Adding > /dev/null fixes it:

system(on_windows? ? "where #{cmd} 2> NUL" : "which #{cmd} > /dev/null")

Other than that, good to go.

@ddfreyne

This comment has been minimized.

Copy link
Member

commented Nov 24, 2013

👍

ddfreyne added a commit that referenced this pull request Nov 24, 2013

@ddfreyne ddfreyne merged commit 025755c into nanoc:release-3.6.x Nov 24, 2013

@raphinesse raphinesse deleted the raphinesse:fix-tests-on-windows branch Nov 24, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.