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 to get script/test to pass on Windows #5526

Merged
merged 6 commits into from Nov 10, 2016

Conversation

Projects
None yet
5 participants
@ashmaroli
Member

ashmaroli commented Nov 2, 2016

  • Fix TestGeneratedSite
  • Fix TestFilters
  • Fix TestSite (only partially)
  • Add new helper method to skip testing symlinks on Windows

--

ref: #5241
/cc @jekyll/windows

TestGeneratedSite: add "|" to preserve newline
adding a pipe character ('|') preserves the formatting of
'expected_output' with a trailing newline bit, in windows.
@ashmaroli

This comment has been minimized.

Show comment
Hide comment
@ashmaroli

ashmaroli Nov 2, 2016

Member

This PR is a refined version of the referenced pull.

Member

ashmaroli commented Nov 2, 2016

This PR is a refined version of the referenced pull.

Show outdated Hide outdated test/test_site.rb
@@ -14,14 +14,20 @@ class TestSite < JekyllUnitTest
should "have an array for plugins if passed as a string" do
site = Site.new(site_configuration({ "plugins_dir" => "/tmp/plugins" }))
assert_equal ["/tmp/plugins"], site.plugins
array = Utils::Platforms.really_windows? ? ["C:/tmp/plugins"] : ["/tmp/plugins"]

This comment has been minimized.

@XhmikosR

XhmikosR Nov 2, 2016

Contributor

Why hardcode this? You should just use the environment variable.

@XhmikosR

XhmikosR Nov 2, 2016

Contributor

Why hardcode this? You should just use the environment variable.

This comment has been minimized.

@ashmaroli

ashmaroli Nov 2, 2016

Member

just use the environment variable

please explain more.

@ashmaroli

ashmaroli Nov 2, 2016

Member

just use the environment variable

please explain more.

This comment has been minimized.

@XhmikosR

XhmikosR Nov 2, 2016

Contributor
C:\Users\XhmikosR\Desktop>echo %TEMP%
C:\Users\XhmikosR\AppData\Local\Temp

C:\Users\XhmikosR\Desktop>echo %TMP%
C:\Users\XhmikosR\AppData\Local\Temp

So, just use the environment variable, any of those two.

@XhmikosR

XhmikosR Nov 2, 2016

Contributor
C:\Users\XhmikosR\Desktop>echo %TEMP%
C:\Users\XhmikosR\AppData\Local\Temp

C:\Users\XhmikosR\Desktop>echo %TMP%
C:\Users\XhmikosR\AppData\Local\Temp

So, just use the environment variable, any of those two.

This comment has been minimized.

@ashmaroli

ashmaroli Nov 2, 2016

Member

the path is not important here actually. This step and the next is to simply test if plugins are correctly detected as specified.
e.g. if were to set plugins_dir => "/apps/bazinga", the array returned would be:

["C:/apps/bazinga"] # on Windows and AppVeyor CI
["/apps/bazinga"] # on Unixoids and Travis CI
@ashmaroli

ashmaroli Nov 2, 2016

Member

the path is not important here actually. This step and the next is to simply test if plugins are correctly detected as specified.
e.g. if were to set plugins_dir => "/apps/bazinga", the array returned would be:

["C:/apps/bazinga"] # on Windows and AppVeyor CI
["/apps/bazinga"] # on Unixoids and Travis CI

This comment has been minimized.

@XhmikosR

XhmikosR Nov 2, 2016

Contributor

I'm just saying, it's bad practice hardcoding paths. Especially when you can use something a lot more reliable to detect the path.

@XhmikosR

XhmikosR Nov 2, 2016

Contributor

I'm just saying, it's bad practice hardcoding paths. Especially when you can use something a lot more reliable to detect the path.

This comment has been minimized.

@ashmaroli

ashmaroli Nov 2, 2016

Member

the more I think about this, I wonder if this is a bug or intended..
when I set, plugins_dir => "/apps/plugins"
I should get a relative path to plugins dir inside my site, or an absolute_path to plugins dir with my site.

@ashmaroli

ashmaroli Nov 2, 2016

Member

the more I think about this, I wonder if this is a bug or intended..
when I set, plugins_dir => "/apps/plugins"
I should get a relative path to plugins dir inside my site, or an absolute_path to plugins dir with my site.

This comment has been minimized.

@ashmaroli

ashmaroli Nov 2, 2016

Member

update: it works as expected, if the leading / is avoided..
set plugins_dir => "apps/bazinga"
then I get site.plugins == [C:/~/apps/bazinga]

@ashmaroli

ashmaroli Nov 2, 2016

Member

update: it works as expected, if the leading / is avoided..
set plugins_dir => "apps/bazinga"
then I get site.plugins == [C:/~/apps/bazinga]

This comment has been minimized.

@ashmaroli

ashmaroli Nov 2, 2016

Member

moving this segment to a dedicated issue ticket..

@ashmaroli

ashmaroli Nov 2, 2016

Member

moving this segment to a dedicated issue ticket..

@parkr

Cool, looking good! Thanks for this.

Out of curiosity, why are we using really_windows? instead of just windows?

Show outdated Hide outdated test/helper.rb
def skip_if_windows(msg = nil)
if Utils::Platforms.really_windows?
msg ||= "Jekyll doesn't currently support Symlinks on Windows"

This comment has been minimized.

@parkr

parkr Nov 2, 2016

Member

This seems like a strange default. Is this only used in symlinks-related code? If not, I'd suggest something like Jekyll does not currently support this feature on Windows.

@parkr

parkr Nov 2, 2016

Member

This seems like a strange default. Is this only used in symlinks-related code? If not, I'd suggest something like Jekyll does not currently support this feature on Windows.

Show outdated Hide outdated test/test_collections.rb
@@ -186,6 +186,9 @@ class TestCollections < JekyllUnitTest
end
should "include the symlinked file from site.source in the list of docs" do
# no support for including symlinked file on Windows
skip_if_windows

This comment has been minimized.

@parkr

parkr Nov 2, 2016

Member

Then here I'd be specific with my message and say skip_if_windows, "Jekyll does not currently support symlinsk on Windows.".

@parkr

parkr Nov 2, 2016

Member

Then here I'd be specific with my message and say skip_if_windows, "Jekyll does not currently support symlinsk on Windows.".

Show outdated Hide outdated test/test_entry_filter.rb
@@ -82,6 +82,9 @@ class TestEntryFilter < JekyllUnitTest
# rubocop:disable Performance/FixedSize
should "include only safe symlinks in safe mode" do
# no support for symlinks on Windows
skip_if_windows

This comment has been minimized.

@parkr

parkr Nov 2, 2016

Member

Same here and everywhere you use this method – make that comment the message instead of a comment so when I run the tests, this information is surfaced.

@parkr

parkr Nov 2, 2016

Member

Same here and everywhere you use this method – make that comment the message instead of a comment so when I run the tests, this information is surfaced.

| - /css/screen.css last edited at #{time_regexp} with extname .css
- /pgp.key last edited at #{time_regexp} with extname .key
- /products.yml last edited at #{time_regexp} with extname .yml
- /symlink-test/symlinked-dir/screen.css last edited at #{time_regexp} with extname .css

This comment has been minimized.

@parkr

parkr Nov 2, 2016

Member

Why is this necessary? Can you paste the test failure in a comment below?

@parkr

parkr Nov 2, 2016

Member

Why is this necessary? Can you paste the test failure in a comment below?

This comment has been minimized.

@ashmaroli

ashmaroli Nov 2, 2016

Member

@parkr 👇

Failure:
TestGeneratedSite#test_: generated sites should print a nice list of static files.  [C:/projects/jekyll/test/test_generated_site.rb:59]
Minitest::Assertion: Expected /- \/css\/screen.css last edited at \d+:\d+ with extname .css
- \/pgp.key last edited at \d+:\d+ with extname .key
- \/products.yml last edited at \d+:\d+ with extname .yml
- \/symlink-test\/symlinked-dir\/screen.css last edited at \d+:\d+ with extname .css
/ to match "\n- /css/screen.css last edited at 23:11 with extname .css\n- /pgp.key last edited at 23:11 with extname .key\n- /products.yml last edited at 23:11 with extname .yml\n- /symlink-test/symlinked-dir last edited at 23:11 with extname \n- /symlink-test/symlinked-file last edited at 23:11 with extname \n".
@ashmaroli

ashmaroli Nov 2, 2016

Member

@parkr 👇

Failure:
TestGeneratedSite#test_: generated sites should print a nice list of static files.  [C:/projects/jekyll/test/test_generated_site.rb:59]
Minitest::Assertion: Expected /- \/css\/screen.css last edited at \d+:\d+ with extname .css
- \/pgp.key last edited at \d+:\d+ with extname .key
- \/products.yml last edited at \d+:\d+ with extname .yml
- \/symlink-test\/symlinked-dir\/screen.css last edited at \d+:\d+ with extname .css
/ to match "\n- /css/screen.css last edited at 23:11 with extname .css\n- /pgp.key last edited at 23:11 with extname .key\n- /products.yml last edited at 23:11 with extname .yml\n- /symlink-test/symlinked-dir last edited at 23:11 with extname \n- /symlink-test/symlinked-file last edited at 23:11 with extname \n".
@@ -175,7 +175,6 @@ def generate(site)
method.call(*args, &block).reverse
end
@site.process
# files in symlinked directories may appear twice

This comment has been minimized.

@parkr

parkr Nov 2, 2016

Member

Maybe say here than symlinked directories are excluded from this list and added in when not Windows.

@parkr

parkr Nov 2, 2016

Member

Maybe say here than symlinked directories are excluded from this list and added in when not Windows.

ashmaroli added some commits Nov 2, 2016

TestSite: add symlinked files only if not Windows
add symlinked files to "sorted_pages" array only when testing on
non-windows platforms.
TestFilters: adjust array size to ignore symlinks
Adjust the size of grouped-items array as it won't include symlinked
pages in Windows.
@ashmaroli

This comment has been minimized.

Show comment
Hide comment
@ashmaroli

ashmaroli Nov 2, 2016

Member

Out of curiosity, why are we using really_windows? instead of just windows?

Because ::Platforms.really_windows? is more strict than ::Platforms.windows?

Member

ashmaroli commented Nov 2, 2016

Out of curiosity, why are we using really_windows? instead of just windows?

Because ::Platforms.really_windows? is more strict than ::Platforms.windows?

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Nov 2, 2016

Member

Because ::Platforms.really_windows? is more strict than ::Platforms.windows?

Why do we want it to be more strict?

Member

parkr commented Nov 2, 2016

Because ::Platforms.really_windows? is more strict than ::Platforms.windows?

Why do we want it to be more strict?

@ashmaroli

This comment has been minimized.

Show comment
Hide comment
@ashmaroli

ashmaroli Nov 2, 2016

Member

Why do we want it to be more strict?

😃 Because currently, symlinks is known to not have native support in Windows env. But then it may work in Windows subsystem on Linux

Member

ashmaroli commented Nov 2, 2016

Why do we want it to be more strict?

😃 Because currently, symlinks is known to not have native support in Windows env. But then it may work in Windows subsystem on Linux

ashmaroli added some commits Nov 2, 2016

add and use 'skip_if_windows' helper method
  - add a new helper method to skip tests if on Windows platform
  - skip those tests that fail due to lack of support for symlinked files
    on Windows.
TestSite: consider dive-letter in Windows
The array of plugins will contain current drive-letter in Windows
@ashmaroli

This comment has been minimized.

Show comment
Hide comment
@ashmaroli

ashmaroli Nov 5, 2016

Member

TODO: Undo Gemfile changes when codeclimate-test-reporter issue is fixed on master

Member

ashmaroli commented Nov 5, 2016

TODO: Undo Gemfile changes when codeclimate-test-reporter issue is fixed on master

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Nov 9, 2016

Member

This is looking excellent!

Member

parkr commented Nov 9, 2016

This is looking excellent!

@parkr

parkr approved these changes Nov 9, 2016

👌

Show outdated Hide outdated Gemfile
@@ -22,7 +22,7 @@ group :test do
gem "cucumber", "~> 2.1"
gem "jekyll_test_plugin"
gem "jekyll_test_plugin_malicious"
gem "codeclimate-test-reporter"
gem "codeclimate-test-reporter", "0.6.0"

This comment has been minimized.

@parkr

parkr Nov 9, 2016

Member

Want to submit a separate PR for this?

@parkr

parkr Nov 9, 2016

Member

Want to submit a separate PR for this?

This comment has been minimized.

@ashmaroli

ashmaroli Nov 9, 2016

Member

@parkr, if you think, going with "~> 0.6.0" is the better way for now, I shall definitely move this to a dedicated PR. Otherwise, this will remain here to have the tests run correctly..

@ashmaroli

ashmaroli Nov 9, 2016

Member

@parkr, if you think, going with "~> 0.6.0" is the better way for now, I shall definitely move this to a dedicated PR. Otherwise, this will remain here to have the tests run correctly..

@parkr parkr changed the title from Fix tests to get them to pass on Windows to Fix tests to get script/test to pass on Windows Nov 10, 2016

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Nov 10, 2016

Member

@jekyllbot: merge +dev

Member

parkr commented Nov 10, 2016

@jekyllbot: merge +dev

@jekyllbot jekyllbot merged commit 3e1fad2 into jekyll:master Nov 10, 2016

1 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

jekyllbot added a commit that referenced this pull request Nov 10, 2016

@ashmaroli ashmaroli deleted the ashmaroli:fix-win-tests branch Nov 11, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment