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

Fail checks when output directory is not present #472

Merged

Conversation

@jugglinmike
Copy link
Contributor

@jugglinmike jugglinmike commented Sep 3, 2014

I recently ran across this behavior with a site I maintain: I wrote a TravisCI configuration file that ran nanoc check ilinks, but I forgot to ensure that Travis actually compiled the site first. For a few weeks, the tests have been passing, and we've been accumulating invalid internal links without knowing it.

@ddfreyne @bobthecow I'm sure you guys will have plenty to say about the code organization here. (For instance, do we really need a new Error subclass? If so, should it be defined elsewhere?) As always, I'm all ears :)

Commit message:

Currently, site checks pass when the output directory is not present.
Ensure that all checks fail in cases where there is no generated site
content to validate.

@ddfreyne
Copy link
Member

@ddfreyne ddfreyne commented Sep 3, 2014

Looks good!

I like having a separate error class, because it makes it easier to handle different errors differently, or change the error message in a single place.

Loading

@ddfreyne
Copy link
Member

@ddfreyne ddfreyne commented Sep 3, 2014

The tests are likely failing due to Gemfile.lock not having 3.7.3 but rather 3.7.2 in it.

Loading

Dir[@site.config[:output_dir] + '/**/*'].select { |f| File.file?(f) }
output_dir = @site.config[:output_dir]
unless File.exists? output_dir
raise Nanoc::Extra::Checking::OutputDirNotFoundError.new(output_dir)
Copy link
Member

@ddfreyne ddfreyne Sep 3, 2014

Choose a reason for hiding this comment

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

Can you remove the extra space here?

Loading

Copy link
Contributor Author

@jugglinmike jugglinmike Sep 3, 2014

Choose a reason for hiding this comment

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

Oof, this was sloppy on my part. Fixing...

Loading

@ddfreyne
Copy link
Member

@ddfreyne ddfreyne commented Sep 3, 2014

If you rebase the branch against release-3.7.x, the tests should work properly on Travis.

Loading

@jugglinmike
Copy link
Contributor Author

@jugglinmike jugglinmike commented Sep 3, 2014

The tests are likely failing due to Gemfile.lock not having 3.7.3 but rather 3.7.2 in it.

Woa, nice detective work, there. I'm on it!

Loading

Currently, site checks pass when the output directory is not present.
Ensure that all checks fail in cases where there is no generated site
content to validate.
@jugglinmike jugglinmike force-pushed the fail-check-for-empty-output branch from 9a897ec to 78be403 Sep 3, 2014
@jugglinmike
Copy link
Contributor Author

@jugglinmike jugglinmike commented Sep 3, 2014

@ddfreyne I incorporated your feedback, and it almost passes the build. Now, instead of seeing build errors across the job matrix, all jobs pass except the jruby-1.7.11 / DISABLE_NOKOGIRI=1. Any ideas?

Loading

@ddfreyne
Copy link
Member

@ddfreyne ddfreyne commented Sep 4, 2014

@jugglinmike The error you’re seeing is something I couldn’t quite debug properly. I am OK with leaving it as-is, because watch is deprecated in favour of guard-nanoc anyway.

Loading

@jugglinmike
Copy link
Contributor Author

@jugglinmike jugglinmike commented Sep 4, 2014

@ddfreyne Alrighty. Is there something more I can do, then? Maybe disabling/removing the Nanoc::CLI::Commands::WatchTest#test_notify test?

Loading

@ddfreyne
Copy link
Member

@ddfreyne ddfreyne commented Sep 5, 2014

@jugglinmike I’ll look into the test at some later time. I might just have to remove it. You can ignore it now though!

Loading

@@ -25,7 +31,11 @@ def add_issue(desc, params = {})
end

def output_filenames
Dir[@site.config[:output_dir] + '/**/*'].select { |f| File.file?(f) }
output_dir = @site.config[:output_dir]
unless File.exists? output_dir
Copy link
Member

@ddfreyne ddfreyne Sep 5, 2014

Choose a reason for hiding this comment

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

Can you add method call parens here, and rename exists to exist?

unless File.exists? output_dir

should be

unless File.exist?(output_dir)

Loading

Copy link
Contributor Author

@jugglinmike jugglinmike Sep 5, 2014

Choose a reason for hiding this comment

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

Sure!

Loading

@jugglinmike
Copy link
Contributor Author

@jugglinmike jugglinmike commented Sep 5, 2014

@ddfreyne I've pushed a commit to address your feedback. Not sure if/when you'd like to squash--say the word and I'll do it for ya

Loading

@ddfreyne
Copy link
Member

@ddfreyne ddfreyne commented Sep 6, 2014

Nope, all fine. Thanks for the contribution!

Loading

ddfreyne added a commit that referenced this issue Sep 6, 2014
Fail checks when output directory is not present
@ddfreyne ddfreyne merged commit a442c7a into nanoc:release-3.7.x Sep 6, 2014
1 check failed
Loading
@ddfreyne ddfreyne added this to the 3.7.4 milestone Sep 6, 2014
@ddfreyne ddfreyne added this to the 3.7.4 milestone Sep 6, 2014
@jugglinmike jugglinmike deleted the fail-check-for-empty-output branch Sep 6, 2014
@jugglinmike
Copy link
Contributor Author

@jugglinmike jugglinmike commented Sep 6, 2014

My pleasure :)

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants