Added tests for Jekyll:StaticFile #3633

Merged
merged 1 commit into from Apr 10, 2015

Conversation

Projects
None yet
4 participants
@mdenhoedt
Contributor

mdenhoedt commented Apr 1, 2015

Added tests for Jekyll:StaticFile this should solve issue #3626.

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Apr 1, 2015

Contributor

Are you sure this is not covered by a higher level test in features/ ?

Contributor

envygeeks commented Apr 1, 2015

Are you sure this is not covered by a higher level test in features/ ?

@mdenhoedt

This comment has been minimized.

Show comment
Hide comment
@mdenhoedt

mdenhoedt Apr 1, 2015

Contributor

The test coverage for StaticFile is already nearly 100%. However, I think it is a good idea to have unit tests for it too, but it's up to you 😄.

Contributor

mdenhoedt commented Apr 1, 2015

The test coverage for StaticFile is already nearly 100%. However, I think it is a good idea to have unit tests for it too, but it's up to you 😄.

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Apr 1, 2015

Contributor

I think it's a good idea to cover it. Anybody else have any objects? /cc @jekyll/core

Contributor

envygeeks commented Apr 1, 2015

I think it's a good idea to cover it. Anybody else have any objects? /cc @jekyll/core

+ def make_dummy_file(file_name)
+ temp_file = File.new(file_name, "w")
+ temp_file.puts("some content")
+ temp_file.close

This comment has been minimized.

@parkr

parkr Apr 4, 2015

Member

Why not:

File.open(filename, "w") do |f|
  f.puts "some content"
end

?

@parkr

parkr Apr 4, 2015

Member

Why not:

File.open(filename, "w") do |f|
  f.puts "some content"
end

?

This comment has been minimized.

@envygeeks

envygeeks Apr 5, 2015

Contributor
File.write(filename, "some content")
@envygeeks

envygeeks Apr 5, 2015

Contributor
File.write(filename, "some content")

This comment has been minimized.

@parkr

parkr Apr 5, 2015

Member

^^ that also works :)

@parkr

parkr Apr 5, 2015

Member

^^ that also works :)

+ context "A StaticFile" do
+ setup do
+ clear_dest
+ @site = Site.new(site_configuration)

This comment has been minimized.

@parkr

parkr Apr 4, 2015

Member

I believe you can use fixture_site here!

@parkr

parkr Apr 4, 2015

Member

I believe you can use fixture_site here!

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Apr 4, 2015

Member

I also think it's a good idea to cover it. :)

Member

parkr commented Apr 4, 2015

I also think it's a good idea to cover it. :)

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Apr 4, 2015

Member

There's definitely an opportunity to DRY up this file (a lot of repetition right now). Would you mind tackling that?

Member

parkr commented Apr 4, 2015

There's definitely an opportunity to DRY up this file (a lot of repetition right now). Would you mind tackling that?

@mdenhoedt

This comment has been minimized.

Show comment
Hide comment
@mdenhoedt

mdenhoedt Apr 4, 2015

Contributor

Thanks for the feedback. I am not an experienced Ruby programmer and I will definitely try to improve my pull request.

Contributor

mdenhoedt commented Apr 4, 2015

Thanks for the feedback. I am not an experienced Ruby programmer and I will definitely try to improve my pull request.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Apr 6, 2015

Member

No problem! Let us know if you have any questions. :)

Member

parkr commented Apr 6, 2015

No problem! Let us know if you have any questions. :)

@parkr parkr merged commit 28a1d24 into jekyll:master Apr 10, 2015

1 check passed

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

parkr added a commit that referenced this pull request Apr 10, 2015

parkr added a commit that referenced this pull request Apr 10, 2015

Merge branch 'delftswa2014-static-file-test'
* delftswa2014-static-file-test:
  DRY up the StaticFile tests a bit. #3633.
  Added tests for Jekyll:StaticFile

parkr added a commit that referenced this pull request Apr 10, 2015

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Apr 10, 2015

Contributor

❤️

Contributor

envygeeks commented Apr 10, 2015

❤️

@jekyll jekyll locked and limited conversation to collaborators Feb 27, 2017

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