-
-
Notifications
You must be signed in to change notification settings - Fork 197
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
Scripts checking #53
Scripts checking #53
Changes from all commits
91c9c7d
d05a6f5
f7497f5
8120dc7
157044e
de74c06
2f8c4aa
860ba6e
9292c70
cb0d8a8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,10 +1,13 @@ | ||
module HTML | ||
class Proofer | ||
class Checks | ||
require File.dirname(__FILE__) + '/check' | ||
require File.dirname(__FILE__) + '/checks/images' | ||
require File.dirname(__FILE__) + '/checks/links' | ||
require File.dirname(__FILE__) + '/checks/favicon' | ||
[ | ||
"/check", | ||
"/checks/images", | ||
"/checks/links", | ||
"/checks/scripts", | ||
"/checks/favicon" | ||
].each { |r| require File.dirname(__FILE__) + r } | ||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,40 @@ | ||
# encoding: utf-8 | ||
|
||
class Script < ::HTML::Proofer::Checkable | ||
|
||
def src | ||
@src unless @src.nil? || @src.empty? | ||
end | ||
|
||
def missing_src? | ||
!src | ||
end | ||
|
||
def blank? | ||
@content.strip.empty? | ||
end | ||
|
||
end | ||
|
||
class Scripts < ::HTML::Proofer::Checks::Check | ||
def run | ||
@html.css('script').each do |s| | ||
script = Script.new s, "script", self | ||
|
||
next if script.ignore? | ||
next unless script.blank? | ||
|
||
# does the script exist? | ||
if script.missing_src? | ||
self.add_issue "script is empty and has no src attribute" | ||
elsif script.remote? | ||
add_to_external_urls script.src | ||
else | ||
self.add_issue("internal script #{script.src} does not exist") unless script.exists? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd rather this be rewritten as: if script.missing_src?
self.add_issue "script is empty and has no src attribute"
else
next if script.ignore?
if script.remote?
add_to_external_urls script.src
else
self.add_issue("internal script #{script.src} does not exist") unless script.exists? |
||
end | ||
|
||
end | ||
|
||
external_urls | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
var x = 1; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
<html> | ||
|
||
<body> | ||
|
||
<script src="http://www.asdo3IRJ395295jsingrkrg4.com/asdo3IRJ.js"></script> | ||
|
||
</body> | ||
|
||
</html> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should I put scripts fixtures in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right here is fine. In another PR I will move the files into directories for a cleanup. I'd rather not that happen in this PR as it will make the diff unfairly big. Thanks for asking! ❤️ |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
<html> | ||
|
||
<body> | ||
|
||
<script>var x = 1;</script> | ||
|
||
</body> | ||
|
||
</html> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
<html> | ||
|
||
<body> | ||
|
||
<script></script> | ||
|
||
</body> | ||
|
||
</html> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
<html> | ||
|
||
<body> | ||
|
||
<script src="doesnotexist.js"></script> | ||
|
||
</body> | ||
|
||
</html> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
<html> | ||
|
||
<body> | ||
|
||
<script src="script.js"></script> | ||
|
||
</body> | ||
|
||
</html> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,35 @@ | ||
require "spec_helper" | ||
|
||
describe "Scripts tests" do | ||
|
||
it "fails for broken external src" do | ||
file = "#{FIXTURES_DIR}/script_broken_external.html" | ||
output = capture_stderr { HTML::Proofer.new(file).run } | ||
output.should match /External link http:\/\/www.asdo3IRJ395295jsingrkrg4.com\/asdo3IRJ.js? failed: 0 Couldn't resolve host name/ | ||
end | ||
|
||
it "works for valid internal src" do | ||
file = "#{FIXTURES_DIR}/script_valid_internal.html" | ||
output = capture_stderr { HTML::Proofer.new(file).run } | ||
output.should == "" | ||
end | ||
|
||
it "fails for missing internal src" do | ||
file = "#{FIXTURES_DIR}/script_missing_internal.html" | ||
output = capture_stderr { HTML::Proofer.new(file).run } | ||
output.should match /doesnotexist.js does not exist/ | ||
end | ||
|
||
it "works for present content" do | ||
file = "#{FIXTURES_DIR}/script_content.html" | ||
output = capture_stderr { HTML::Proofer.new(file).run } | ||
output.should == "" | ||
end | ||
|
||
it "fails for absent content" do | ||
file = "#{FIXTURES_DIR}/script_content_absent.html" | ||
output = capture_stderr { HTML::Proofer.new(file).run } | ||
output.should match /script is empty and has no src attribute/ | ||
end | ||
|
||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should respect the user in the first place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough. 👍