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

Travis : Check site with html-proofer #22

Closed
wants to merge 1 commit into from
Closed

Conversation

johnhaddon
Copy link
Member

No description provided.

@johnhaddon
Copy link
Member Author

@medubelko, this is an attempt to add CI to the website, so we don't get into the situation we've just had with broken links. Currently it fails because there are still outstanding problems with the site. Could you take a look at fixing them please?

You can run the tests locally with this :

bundle exec jekyll build && bundle exec htmlproofer ./_site --only-4xx --check-html --empty-alt-ignore

Really we should remove the --empty-alt-ignore flag, but I've used it for now to silence a whole load of errors about missing alt text...

@andrewkaufman
Copy link
Contributor

Are we sure this is working as expected? It seems to be claiming gafferhq.org/news/ doesn't exist... or am I reading that error log wrong?

@medubelko
Copy link
Contributor

medubelko commented Oct 10, 2018

I tested this today.

html-proofer uses Nokogiri for HTML parsing, and it doesn't support full HTML5 compliance.

In my error-fix PR, I had to make the following adjustments to nominal HTML so that the proofer would behave:

  • Remove <script> tags inside <p> tags (bananas)
  • Convert relative paths that didn't start with .. into absolute paths (gong show)

So, it fails HTML that had no errors with the official W3C validator. I hope it doesn't flag too much other valid code...

@johnhaddon
Copy link
Member Author

I'm not wed to any particular solution, but it's clear that we need CI for the website. Key links have been broken twice recently, and we haven't even known about it until someone pointed it out. It seems you can pass --checks-to-ignore HtmlCheck,ScriptCheck,ImageCheck to restrict the checks to just links, which gets rid of a whole host of errors - maybe that's an option.

@medubelko
Copy link
Contributor

Merged my HTML fixes. @johnhaddon Let's see what Travis reports now. I think you'll need to make a dummy commit to trigger a re-test.

@andrewkaufman
Copy link
Contributor

apparently we have another broken link... should we merge this now so we catch these things more easily?

@medubelko
Copy link
Contributor

medubelko commented Oct 22, 2018

If you were referring to the link in GafferHQ/gaffer#2856, Travis wouldn't have caught it, since everything in the public documentation is part of https://github.com/gafferHQ/documentation, copied to the gh-pages separately. I'll check to see why I didn't notice the dead link in the Sphinx log.

In any case, I'll see about this repo's Travis config today.

@johnhaddon
Copy link
Member Author

I'll check to see why I didn't notice the dead link in the Sphinx log.

Rather than manual checks, can we make Sphinx error on missing internal links?

@medubelko
Copy link
Contributor

medubelko commented Oct 27, 2018

I re-tested with my HTML fixes. As suspected, the proofer is still throwing errors due to {{ site.baseurl }} in the nav links in header.html, multiplied by the number of blog posts we have. I'll have to think about whether there's a good way around that.

@medubelko
Copy link
Contributor

Rather than manual checks, can we make Sphinx error on missing internal links?

Doesn't seem to be an option yet. External links, yes, but not internal. A quick search brought up this issue. The -n nitpicky option can check ReST references, but not links. I also suspect that even if it did, the Markdown conversion would prevent it from working properly.

@medubelko
Copy link
Contributor

medubelko commented Dec 21, 2018

Fixed in #30.

html-proofer has an undocumented soft dependency on the jekyll-relative-links gem, which needs Jekyll 3.3/Ruby 2.3.3. In my PR, I bumped the rvm to 2.3.7 (matching IE*), which fixes the navigation link issue.

Here's the documentation for how I replicated the Travis build locally: https://github.com/GafferHQ/gafferhq.github.io/blob/master/VALIDATING.md

(* Our gem versions lag behind the 2.3.7 lineage. Jekyll is 3.1.6, github-pages is 82, etc. I'll take this opportunity to again mention that as a whole, this setup is far behind the GH-Pages dependencies...)

@medubelko medubelko closed this Dec 21, 2018
@medubelko medubelko deleted the travisSetup branch December 21, 2018 17:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants