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

Grunt W3C Validation #470

Merged
merged 25 commits into from
May 16, 2017
Merged

Grunt W3C Validation #470

merged 25 commits into from
May 16, 2017

Conversation

nvasileiadis
Copy link
Contributor

The validation of the project's output markup HTML with W3C standards validator has been added to and updated the Grunt workflow.

This helps to maintain an updated bundle of dependencies among different branches, devs and machines.
This folder will contain any errors, in a formated and timestamped way, after the grunt is run.
Those two files added are containing a raw output of the validation run and are overwritten every time.
@nvasileiadis
Copy link
Contributor Author

There is a need for a directories check and addition if anything is missing.

@nvasileiadis nvasileiadis self-assigned this Jan 24, 2017
@codecov-io
Copy link

codecov-io commented Jan 24, 2017

Codecov Report

Merging #470 into develop will decrease coverage by 0.04%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #470      +/-   ##
===========================================
- Coverage     98.4%   98.36%   -0.05%     
===========================================
  Files           16       15       -1     
  Lines          627      611      -16     
===========================================
- Hits           617      601      -16     
  Misses          10       10
Impacted Files Coverage Δ
js/DisableUiComponent.js

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4a72a5d...eb9e212. Read the comment docs.

Gruntfile.js Outdated
},
files: {
src: ['dist/views/*.html',
'emails/dist/*.html',
Copy link
Member

Choose a reason for hiding this comment

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

Tabbing looks a bit odd here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

Gruntfile.js Outdated
reset: grunt.option('reset') || false,
stoponerror: false,
maxTry: 3,
relaxerror: ['Bad value X-UA-Compatible for attribute http-equiv on element meta.'], //ignores these errors
Copy link
Member

Choose a reason for hiding this comment

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

Minor point, but need space after //

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@@ -26,3 +26,5 @@ pulsar.zip
/fonts/_config.fonts.scss

/.csscomb.json
/validation-status.json
Copy link
Member

Choose a reason for hiding this comment

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

These files will also need adding to the .gitattributes file as export-ignore rules so that Continuum products can ignore them

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added

Gruntfile.js Outdated
generateReport: true,
errorHTMLRootDir: "w3cErrorFolder",
useTimeStamp: true,
errorTemplate: "w3cErrorFolder/w3c_validation_error_Template.html"
Copy link
Member

Choose a reason for hiding this comment

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

This might be more appropriate to put in the /tests directory, /tests/validation perhaps?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Relocated as advised

Gruntfile.js Outdated
maxTry: 3,
relaxerror: ['Bad value X-UA-Compatible for attribute http-equiv on element meta.'], //ignores these errors
generateReport: true,
errorHTMLRootDir: "w3cErrorFolder",
Copy link
Member

Choose a reason for hiding this comment

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

Wherever the output is placed, we'll need to add it to the .gitignore file so it's not committed to the repository

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added

Gruntfile.js Outdated
@@ -494,16 +513,19 @@ module.exports = function(grunt) {
]
});

grunt.loadNpmTasks('grunt-w3c-html-validation');
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't need this, it's handled by matchdep at the bottom of the gruntfile

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Deleted

Gruntfile.js Outdated
@@ -494,16 +513,19 @@ module.exports = function(grunt) {
]
});

grunt.loadNpmTasks('grunt-w3c-html-validation');

grunt.registerTask('default', [
Copy link
Member

Choose a reason for hiding this comment

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

How long does the validation task take? Is it something we want to run on the default task, or something to roll into a testing/pre-release task?

Copy link
Contributor Author

@nvasileiadis nvasileiadis Jan 26, 2017

Choose a reason for hiding this comment

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

It takes some time, moved from default to separate task. Can be run with 'grunt validate' at will

This is for W3C Validation grunt task.
The file produced and the directory used by W3C Validation.
Changes:
1. Move error reports directory and template to /tests/validation
2. Correct indentation and spacing
3. Remove obsolete loadNpmTasks
4. Make ‘validate’ a separate task
Changed the gitingore and gitattibutes to include only the folders and files generated after the task run. The previous settings were ignoring the error template.
Copy link
Member

@Stanton Stanton left a comment

Choose a reason for hiding this comment

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

Couple of new things now I've had chance to check out and run the task.

Gruntfile.js Outdated
},
files: {
src: ['dist/views/*.html',
'emails/dist/*.html',
Copy link
Member

Choose a reason for hiding this comment

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

On second thoughts I don't think validating emails is much use as most of this is automatically generated based on email client quirks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Emails have been removed from the related settings in gruntfile.

Gruntfile.js Outdated
errorTemplate: "tests/validation/w3c_validation_error_Template.html"
},
files: {
src: ['dist/views/*.html',
Copy link
Member

Choose a reason for hiding this comment

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

I think we should list the relevant lexicon urls we'd want to validate rather than just the .html files e have in /dist, this will let us validate against our Twig output.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean /dist/views/pulsar? Because all the other files I see are assets. If this is the only folder with the html and twig parts I will change and commit it.

js/casper.js Outdated
fs.write('dist/htmloutput/page.html', this.getPageContent(), 'w');
});

casper.run(function() {

Choose a reason for hiding this comment

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

'casper' is not defined.

js/casper.js Outdated
@@ -0,0 +1,9 @@
var fs = require('fs');

casper.start('http://google.com', function() {

Choose a reason for hiding this comment

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

'casper' is not defined.

js/casper.js Outdated

casper.run(function() {
this.exit();
})

Choose a reason for hiding this comment

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

Missing semicolon.

js/casper.js Outdated
});

casper.run(function() {
this.exit();

Choose a reason for hiding this comment

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

Missing "use strict" statement.

js/casper.js Outdated
var fs = require('fs');

casper.start('http://google.com', function() {
fs.write('dist/htmloutput/page.html', this.getPageContent(), 'w');

Choose a reason for hiding this comment

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

Missing "use strict" statement.

Now it should output properly different report for each page.
js/casper.js Outdated

for (;current < end;) {
(function(cntr) {
casper.thenOpen('http://192.168.13.37/' + links[cntr] + '', function() {

Choose a reason for hiding this comment

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

'casper' is not defined.

js/casper.js Outdated
var end = links.length;

for (;current < end;) {
(function(cntr) {

Choose a reason for hiding this comment

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

Don't make functions within a loop.

js/casper.js Outdated

for (;current < end;) {
(function(cntr) {
casper.thenOpen('http://192.168.13.37/' + links[cntr] + '', function() {

Choose a reason for hiding this comment

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

'casper' is not defined.

js/casper.js Outdated

// Get Page Name from URL
for (var i = 0; i < links.length; i++) {
if (links[i].match(regex) != null ) { // Remove null items that didn't pass the regex

Choose a reason for hiding this comment

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

Expected '!==' and instead saw '!='.

js/casper.js Outdated

// Get Page Name from URL
for (var i = 0; i < newlinks.length; i++) {
if (newlinks[i].match(regex) != null ) { // Remove null items that didn't pass the regex

Choose a reason for hiding this comment

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

Expected '!==' and instead saw '!='.

Copy link
Contributor

Choose a reason for hiding this comment

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

This could be simplified further to if (newlinks[i].match(regex )) as we'll get an array with the matching regex query if we've got a match, this will also keep the hound at bay.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Refactored

js/casper.js Outdated
};

// Get Page Name from URL
for (var i = 0; i < newlinks.length; i++) {

Choose a reason for hiding this comment

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

'i' is already defined.

js/casper.js Outdated
if (links[i].indexOf('#') == -1) {
newlinks.push(links[i]);
};
};

Choose a reason for hiding this comment

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

Unnecessary semicolon.

js/casper.js Outdated
for (var i = 0; i < links.length; i++) {
if (links[i].indexOf('#') == -1) {
newlinks.push(links[i]);
};

Choose a reason for hiding this comment

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

Unnecessary semicolon.

js/casper.js Outdated

// Remove links within the same page
for (var i = 0; i < links.length; i++) {
if (links[i].indexOf('#') == -1) {

Choose a reason for hiding this comment

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

Expected '===' and instead saw '=='.

Copy link
Contributor

Choose a reason for hiding this comment

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

There's also an extra space before ==, I agree with the hound on this one, always best to be explicit with operators.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

An extra '=' was added

@nvasileiadis
Copy link
Contributor Author

Closes #469

@nvasileiadis nvasileiadis reopened this May 16, 2017
@nvasileiadis nvasileiadis merged commit 555b3f4 into develop May 16, 2017
@nvasileiadis
Copy link
Contributor Author

Closes #469

@nvasileiadis nvasileiadis deleted the grunt-w3c-validation branch May 16, 2017 09:51
@nvasileiadis nvasileiadis restored the grunt-w3c-validation branch May 16, 2017 10:04
@jadu jadu deleted a comment from houndci-bot Oct 23, 2017
@jadu jadu deleted a comment from houndci-bot Oct 23, 2017
@jadu jadu deleted a comment from houndci-bot Oct 23, 2017
@jadu jadu deleted a comment from houndci-bot Oct 23, 2017
@jadu jadu deleted a comment from houndci-bot Oct 23, 2017
@jadu jadu deleted a comment from houndci-bot Oct 23, 2017
@jadu jadu deleted a comment from houndci-bot Oct 23, 2017
@jadu jadu deleted a comment from houndci-bot Oct 23, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants