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

fix: banner comment in generated axe files #1112

Merged
merged 2 commits into from
Aug 30, 2018
Merged

Conversation

jeeyyy
Copy link
Contributor

@jeeyyy jeeyyy commented Aug 29, 2018

  • This PR ensures, generation of banner comments on axe.js and axe.min.js files as a part of grunt build step.
  • grunt-contrib-uglify > 2.x has some API changes which replaced preserveComments with options.comments which is now adapted here.

Closes issue:

Reviewer checks

Required fields, to be filled out by PR reviewer(s)

  • Follows the commit message policy, appropriate for next version
  • Has documentation updated, a DU ticket, or requires no documentation change
  • Includes new tests, or was unnecessary
  • Code is reviewed for security by: << Name here >>

@jeeyyy jeeyyy requested a review from a team as a code owner August 29, 2018 09:49
@dylanb
Copy link
Contributor

dylanb commented Aug 29, 2018

@JKODU Any way we can add an integration test for this? Seems like this keeps happening and it would be good to catch it in the build pre-publish.

@stephenmathieson
Copy link
Member

An integration test seems like overkill to me, but this should work:

#!/bin/bash

e="/*! aXe"
for file in axe.js axe.min.js; do
  src=$(cat $file)
  if [[ $src != $e* ]]; then
    echo "$file does not contain required comment" >&2 
    exit 1
  fi
done 

@jeeyyy
Copy link
Contributor Author

jeeyyy commented Aug 29, 2018

@dylanb @stephenmathieson

Sure thing, can be done, but I will tackle that one (writing tests) separately as I need to write some tests to ensure these files are generated in the first place, before verifying the contents for comments etc.

This was the agreed solution we came up in the retrospective to have tests around generated files and their timestamps, to avoid the issue we had with releasing axe earlier this week without generated files. @WilcoFiers and I believe this needs to be across all the projects, so will get around to that later.

Copy link
Member

@stephenmathieson stephenmathieson left a comment

Choose a reason for hiding this comment

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

LGTM

@WilcoFiers
Copy link
Contributor

@JKODU In thinking some more about build processes. I don't think we need prepublish tests for all projects, just the ones with complicated build steps. For some it might just be enough to check that the dist directory is there. Either way, please create a ticket to add a prepublish test to axe-core.

@jeeyyy
Copy link
Contributor Author

jeeyyy commented Aug 30, 2018

Issue created to ensure build generated files exist - #1117

@jeeyyy jeeyyy merged commit e4788bf into develop Aug 30, 2018
@jeeyyy jeeyyy deleted the fix-banner-comments branch August 30, 2018 10:55
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.

4 participants