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

test: require new release rule help docs to be active before creating release #1700

Merged
merged 6 commits into from
Jul 15, 2019

Conversation

straker
Copy link
Contributor

@straker straker commented Jul 11, 2019

This should block a release pr (a pr into master branch only) if the newest axe version rule help docs are not active.

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: Stephen

@straker straker requested a review from a team as a code owner July 11, 2019 16:11
var assert = require('assert');
var packageJSON = require(path.join(__dirname, '../package.json'));

var version = packageJSON.version.substr(
Copy link
Member

Choose a reason for hiding this comment

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

This looks a little more complicated and error-prone than necessary. What about:

var [major,minor] = packageJSON.version.split('.')
var url = `https://dequeuniversity.com/rules/axe/${major}.${minor}`
// ...

Copy link
Member

Choose a reason for hiding this comment

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

Also, since this test is run in a Node.js context, we can use "modern" JS syntax. We don't have PhantomJS getting in the way here.

Copy link
Contributor Author

@straker straker Jul 11, 2019

Choose a reason for hiding this comment

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

Except that our eslint blocks all es6 syntax in the test dir.

Copy link
Member

Choose a reason for hiding this comment

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

Bummer OK

WilcoFiers
WilcoFiers previously approved these changes Jul 12, 2019
@WilcoFiers WilcoFiers changed the title tests: require new release rule help docs to be active before creating release test: require new release rule help docs to be active before creating release Jul 12, 2019
@straker straker dismissed stale reviews from WilcoFiers and stephenmathieson via 92851fc July 12, 2019 14:32
Copy link
Contributor

@jeeyyy jeeyyy left a comment

Choose a reason for hiding this comment

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

LGTM. Nitpick, the test/* folder allows for es6 syntax.

@stephenmathieson
Copy link
Member

LGTM. Nitpick, the test/* folder allows for es6 syntax.

Per #1700 (comment), we'd need to update ESLint to allow for it.

@straker straker merged commit e9f9c18 into develop Jul 15, 2019
@straker straker deleted the testRuleHelpVersion branch July 15, 2019 14:06
WilcoFiers pushed a commit that referenced this pull request Jul 22, 2019
… release (#1700)

* tests: require new release rule help docs to be active before creating release

* use underscores

* update comment

* update comment

* use split

* add test_rule_help_version to workflows
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