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

Create 'tmp' dir for test_tags if it doesn't exist #3609

Merged
merged 1 commit into from Mar 23, 2015

Conversation

Projects
None yet
3 participants
@nickburlett
Contributor

nickburlett commented Mar 22, 2015

Rather than use script/test to create the tmp directory, create it in a setup block for the appropriate context in the TestTags test.

@nickburlett

This comment has been minimized.

Show comment
Hide comment
@nickburlett

nickburlett Mar 22, 2015

Contributor

/cc #3590

Contributor

nickburlett commented Mar 22, 2015

/cc #3590

@nickburlett

This comment has been minimized.

Show comment
Hide comment
@nickburlett

nickburlett Mar 22, 2015

Contributor

@parkr ... crud, I missed a case that doesn't come up in any of the orderings tried on my system. Two questions:

  1. Is there a way to pass the seed value to the test script or the rake command? I can't find one that works.
  2. Is there a better way to squash the commit that fixes the test failure without making a new branch?
Contributor

nickburlett commented Mar 22, 2015

@parkr ... crud, I missed a case that doesn't come up in any of the orderings tried on my system. Two questions:

  1. Is there a way to pass the seed value to the test script or the rake command? I can't find one that works.
  2. Is there a better way to squash the commit that fixes the test failure without making a new branch?
@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Mar 23, 2015

Contributor
  1. What seed value are you trying to pass? "tmp" is hardcoded so there is no reason to pass that?
  2. git rebase -i after you make a commit and then +branch when you push and it will rebase and update the current pull request with the latest data from your feature branch.

If you aren't using a feature branch, now you know why they ask us to use feature branches 😉

Contributor

envygeeks commented Mar 23, 2015

  1. What seed value are you trying to pass? "tmp" is hardcoded so there is no reason to pass that?
  2. git rebase -i after you make a commit and then +branch when you push and it will rebase and update the current pull request with the latest data from your feature branch.

If you aren't using a feature branch, now you know why they ask us to use feature branches 😉

@nickburlett

This comment has been minimized.

Show comment
Hide comment
@nickburlett

nickburlett Mar 23, 2015

Contributor

@envygeeks

  1. I'm trying to pass a seed value because it seems that the ordering of tests being run depends on it. There are two tests that require the tmp directory to be made, but in this version I accidentally made it so only one of the two creates the tmp directory, making the ordering important.
  2. I had read that git rebase -i was not a good idea if you had already pushed to a public repository. But I guess since no one is likely to have pulled from my patch branch, it shouldn't be a problem

Also,

There is no need for File.directory? when doing FileUtils.mkdir_p because it silently ignores existing folders.

Good point.

I'll commit the fix, rebase, and push.

Contributor

nickburlett commented Mar 23, 2015

@envygeeks

  1. I'm trying to pass a seed value because it seems that the ordering of tests being run depends on it. There are two tests that require the tmp directory to be made, but in this version I accidentally made it so only one of the two creates the tmp directory, making the ordering important.
  2. I had read that git rebase -i was not a good idea if you had already pushed to a public repository. But I guess since no one is likely to have pulled from my patch branch, it shouldn't be a problem

Also,

There is no need for File.directory? when doing FileUtils.mkdir_p because it silently ignores existing folders.

Good point.

I'll commit the fix, rebase, and push.

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Mar 23, 2015

Contributor
  1. Try moving the setup into the base parent describe. If setup is not available there, try before without any arguments (as I do believe that is the Minitest::Spec way.)
  2. Normally it's not okay to rebase on a public repo (unless all are well aware of the history rewrite - and we are) but in the case of pull requests sometimes we do like to rebase and often if we want to squash we do ask for a rebase (quite often I ask people to squash multiple commits into a single more readable history chain) because you are right, it's no big deal on a pull request because your history is independent of ours until we merge but we would never rebase on Jekyll because that could cause chaos.
Contributor

envygeeks commented Mar 23, 2015

  1. Try moving the setup into the base parent describe. If setup is not available there, try before without any arguments (as I do believe that is the Minitest::Spec way.)
  2. Normally it's not okay to rebase on a public repo (unless all are well aware of the history rewrite - and we are) but in the case of pull requests sometimes we do like to rebase and often if we want to squash we do ask for a rebase (quite often I ask people to squash multiple commits into a single more readable history chain) because you are right, it's no big deal on a pull request because your history is independent of ours until we merge but we would never rebase on Jekyll because that could cause chaos.
@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Mar 23, 2015

Contributor

OH, it looks like we aren't fully utilizing minitest spec, we are hybrid, so put the setup at the very top of the class. I forgot we were still hybrid until I finished going through the tests.

Contributor

envygeeks commented Mar 23, 2015

OH, it looks like we aren't fully utilizing minitest spec, we are hybrid, so put the setup at the very top of the class. I forgot we were still hybrid until I finished going through the tests.

@nickburlett

This comment has been minimized.

Show comment
Hide comment
@nickburlett

nickburlett Mar 23, 2015

Contributor

@envygeeks So something like:

diff --git a/test/test_tags.rb b/test/test_tags.rb
index 0460f3f..945cc2d 100644
--- a/test/test_tags.rb
+++ b/test/test_tags.rb
@@ -4,6 +4,10 @@ require 'helper'

 class TestTags < JekyllUnitTest

+  def setup
+    FileUtils.mkdir_p("tmp")
+  end
+
   def create_post(content, override = {}, converter_class = Jekyll::Converters::Markdown)
     site = fixture_site({"highlighter" => "rouge"}.merge(override))
Contributor

nickburlett commented Mar 23, 2015

@envygeeks So something like:

diff --git a/test/test_tags.rb b/test/test_tags.rb
index 0460f3f..945cc2d 100644
--- a/test/test_tags.rb
+++ b/test/test_tags.rb
@@ -4,6 +4,10 @@ require 'helper'

 class TestTags < JekyllUnitTest

+  def setup
+    FileUtils.mkdir_p("tmp")
+  end
+
   def create_post(content, override = {}, converter_class = Jekyll::Converters::Markdown)
     site = fixture_site({"highlighter" => "rouge"}.merge(override))
Create 'tmp' dir for test_tags if it doesn't exist
Rather than use script/test to create the tmp directory, create it in a setup block for the appropriate context in the `TestTags` test.
@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Mar 23, 2015

Contributor

Yessum ❤️

Contributor

envygeeks commented Mar 23, 2015

Yessum ❤️

@nickburlett

This comment has been minimized.

Show comment
Hide comment
@nickburlett

nickburlett Mar 23, 2015

Contributor

@envygeeks Done.

@parkr I think/hope the goose-chase is over...

Contributor

nickburlett commented Mar 23, 2015

@envygeeks Done.

@parkr I think/hope the goose-chase is over...

envygeeks added a commit that referenced this pull request Mar 23, 2015

Merge pull request #3609 from nickburlett/patch/issue-3588-squashed
Create 'tmp' dir for test_tags if it doesn't exist

@envygeeks envygeeks merged commit 55c890f into jekyll:master Mar 23, 2015

1 check passed

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

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Mar 23, 2015

Contributor

❤️

Contributor

envygeeks commented Mar 23, 2015

❤️

@nickburlett nickburlett deleted the nickburlett:patch/issue-3588-squashed branch Mar 23, 2015

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Mar 23, 2015

Member

🎉 !

Member

parkr commented Mar 23, 2015

🎉 !

nickburlett added a commit to nickburlett/jekyll that referenced this pull request Mar 24, 2015

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