PasswordRulesParser.js should have unit tests

[prabhu]

This is a good initiative. But there are:

• no unit tests
• no comments so not able to follow several flows. For eg: what is the method _parsePasswordRequiredOrAllowedPropertyValue doing?
• Single line if conditions are lacking braces. Did we forget goto fail?

Hopefully the code will get improved over time.

[rmondello]

This is a good initiative.

Thanks!

But there are:

• no unit tests

I have a test suite written for the parser, but I haven’t figured out how to make it or something like it run on Github yet. I’ll inline it at the end of this comment, and maybe you’ll have ideas on how we can integrate it into the project. It’s a Ruby script that has a dependency on execjs.

• no comments so not able to follow several flows. For eg: what is the method _parsePasswordRequiredOrAllowedPropertyValue doing?

Fair! I personally might not be able to get around to patching this up soon, but PRs are welcome!

• Single line if conditions are lacking braces. Did we forget goto fail?

This is a holdover from the WebKit project’s style. I’d agree with you that we should deviate from it here. If you’d like to send in a PR for that, I’d happily review it. Otherwise, I’ll get around to that sometime!

Hopefully the code will get improved over time.

#!/usr/bin/env ruby

PATH_TO_CONTAINING_DIRECTORY = File.expand_path(File.dirname(__FILE__))
$LOAD_PATH.unshift "#{PATH_TO_CONTAINING_DIRECTORY}/vendor/execjs-2.6.0/lib"

require 'execjs'

input_path = ARGV.shift

if !input_path
  STDERR.puts "Need to specify both an input parser path."
  exit 1
end

password_rules_parser_javascript = File.read input_path
javaScriptContext = ExecJS.compile password_rules_parser_javascript

DEFAULT_RULES = '[{"_name"=>"allowed", "value"=>[{"_name"=>"ascii-printable"}]}]'

input_and_expected_results = [
  ["", DEFAULT_RULES],
  ["    required: upper", '[{"_name"=>"required", "value"=>[{"_name"=>"upper"}]}, {"_name"=>"allowed", "value"=>[{"_name"=>"upper"}]}]'],
  ["    required: upper;", '[{"_name"=>"required", "value"=>[{"_name"=>"upper"}]}, {"_name"=>"allowed", "value"=>[{"_name"=>"upper"}]}]'],
  ["    required: upper             ", '[{"_name"=>"required", "value"=>[{"_name"=>"upper"}]}, {"_name"=>"allowed", "value"=>[{"_name"=>"upper"}]}]'],
  ["required: uPPeR", '[{"_name"=>"required", "value"=>[{"_name"=>"upper"}]}, {"_name"=>"allowed", "value"=>[{"_name"=>"upper"}]}]'],
  ["required:upper", '[{"_name"=>"required", "value"=>[{"_name"=>"upper"}]}, {"_name"=>"allowed", "value"=>[{"_name"=>"upper"}]}]'],
  ["required:     upper", '[{"_name"=>"required", "value"=>[{"_name"=>"upper"}]}, {"_name"=>"allowed", "value"=>[{"_name"=>"upper"}]}]'],
  ["allowed:upper", '[{"_name"=>"allowed", "value"=>[{"_name"=>"upper"}]}]'],
  ["allowed:     upper", '[{"_name"=>"allowed", "value"=>[{"_name"=>"upper"}]}]'],
  ["required: upper, [AZ];", '[{"_name"=>"required", "value"=>[{"_name"=>"upper"}]}, {"_name"=>"allowed", "value"=>[{"_name"=>"upper"}]}]'],
  ["required: upper; allowed: upper; allowed: lower", '[{"_name"=>"required", "value"=>[{"_name"=>"upper"}]}, {"_name"=>"allowed", "value"=>[{"_name"=>"upper"}, {"_name"=>"lower"}]}]'],
  ["max-consecutive:      5", '[{"_name"=>"allowed", "value"=>[{"_name"=>"ascii-printable"}]}, {"_name"=>"max-consecutive", "value"=>5}]'],
  ["max-consecutive:5", '[{"_name"=>"allowed", "value"=>[{"_name"=>"ascii-printable"}]}, {"_name"=>"max-consecutive", "value"=>5}]'],
  ["max-consecutive:      5", '[{"_name"=>"allowed", "value"=>[{"_name"=>"ascii-printable"}]}, {"_name"=>"max-consecutive", "value"=>5}]'],
  ["max-consecutive: 5; max-consecutive: 3", '[{"_name"=>"allowed", "value"=>[{"_name"=>"ascii-printable"}]}, {"_name"=>"max-consecutive", "value"=>3}]'],
  # The lower number for max-consecutive wins.
  ["max-consecutive: 3; max-consecutive: 5", '[{"_name"=>"allowed", "value"=>[{"_name"=>"ascii-printable"}]}, {"_name"=>"max-consecutive", "value"=>3}]'],
  ["max-consecutive: 3; max-consecutive: 1; max-consecutive: 5", '[{"_name"=>"allowed", "value"=>[{"_name"=>"ascii-printable"}]}, {"_name"=>"max-consecutive", "value"=>1}]'],
  ["required: ascii-printable; max-consecutive: 5; max-consecutive: 3", '[{"_name"=>"required", "value"=>[{"_name"=>"ascii-printable"}]}, {"_name"=>"allowed", "value"=>[{"_name"=>"ascii-printable"}]}, {"_name"=>"max-consecutive", "value"=>3}]'],
  ["required: [*&^]; allowed: upper", '[{"_name"=>"required", "value"=>[{"_characters"=>["&", "*", "^"]}]}, {"_name"=>"allowed", "value"=>[{"_name"=>"upper"}, {"_characters"=>["&", "*", "^"]}]}]'],
  ["required: [*&^ABC]; allowed: upper", '[{"_name"=>"required", "value"=>[{"_characters"=>["A", "B", "C", "&", "*", "^"]}]}, {"_name"=>"allowed", "value"=>[{"_name"=>"upper"}, {"_characters"=>["&", "*", "^"]}]}]'],
  ["required: unicode; required: digit", '[{"_name"=>"required", "value"=>[{"_name"=>"unicode"}]}, {"_name"=>"required", "value"=>[{"_name"=>"digit"}]}, {"_name"=>"allowed", "value"=>[{"_name"=>"unicode"}]}]'],
  ["required: ; required: upper", '[{"_name"=>"required", "value"=>[{"_name"=>"upper"}]}, {"_name"=>"allowed", "value"=>[{"_name"=>"upper"}]}]'],
  # You cannot make rules on unicode characters themselves. They're dropped.
  ["allowed: [供应商责任进展]", DEFAULT_RULES],
  ["allowed: [供应A商B责任C进展]", '[{"_name"=>"allowed", "value"=>[{"_characters"=>["A", "B", "C"]}]}]'],

  ["required: upper; allowed: upper; allowed: lower; minlength: 12; maxlength: 73;", '[{"_name"=>"required", "value"=>[{"_name"=>"upper"}]}, {"_name"=>"allowed", "value"=>[{"_name"=>"upper"}, {"_name"=>"lower"}]}, {"_name"=>"minlength", "value"=>12}, {"_name"=>"maxlength", "value"=>73}]'],
  ["required: upper; allowed: upper; allowed: lower; maxlength: 73; minlength: 12;", '[{"_name"=>"required", "value"=>[{"_name"=>"upper"}]}, {"_name"=>"allowed", "value"=>[{"_name"=>"upper"}, {"_name"=>"lower"}]}, {"_name"=>"minlength", "value"=>12}, {"_name"=>"maxlength", "value"=>73}]'],
  ["required: upper; allowed: upper; allowed: lower; maxlength: 73", '[{"_name"=>"required", "value"=>[{"_name"=>"upper"}]}, {"_name"=>"allowed", "value"=>[{"_name"=>"upper"}, {"_name"=>"lower"}]}, {"_name"=>"maxlength", "value"=>73}]'],
  ["required: upper; allowed: upper; allowed: lower; minlength: 12;", '[{"_name"=>"required", "value"=>[{"_name"=>"upper"}]}, {"_name"=>"allowed", "value"=>[{"_name"=>"upper"}, {"_name"=>"lower"}]}, {"_name"=>"minlength", "value"=>12}]'],
  ["minlength: 12; minlength: 7; minlength: 23", '[{"_name"=>"allowed", "value"=>[{"_name"=>"ascii-printable"}]}, {"_name"=>"minlength", "value"=>23}]'],

  ["minlength: 12; maxlength: 17; minlength: 10", '[{"_name"=>"allowed", "value"=>[{"_name"=>"ascii-printable"}]}, {"_name"=>"minlength", "value"=>12}, {"_name"=>"maxlength", "value"=>17}]'],
  ["allowed: upper,,", DEFAULT_RULES],
  ["allowed: upper,;", DEFAULT_RULES],
  ["allowed: upper [a]", DEFAULT_RULES],
  ["dummy: upper", DEFAULT_RULES],
  ["upper: lower", DEFAULT_RULES],
  ["max-consecutive: [ABC]", DEFAULT_RULES],
  ["max-consecutive: upper", DEFAULT_RULES],
  ["max-consecutive: 1+1", DEFAULT_RULES],
  ["max-consecutive: 供", DEFAULT_RULES],
  ["required: 1", DEFAULT_RULES],
  ["required: 1+1", DEFAULT_RULES],
  ["required: 供", DEFAULT_RULES],
  ["required: A", DEFAULT_RULES],
  ["required: required: upper", DEFAULT_RULES],
  ["allowed: 1", DEFAULT_RULES],
  ["allowed: 1+1", DEFAULT_RULES],
  ["allowed: 供", DEFAULT_RULES],
  ["allowed: A", DEFAULT_RULES],
  ["allowed: allowed: upper", DEFAULT_RULES],

  # Whitespace
  ["required:         digit           ;                        required: [a-];", '[{"_name"=>"required", "value"=>[{"_name"=>"digit"}]}, {"_name"=>"required", "value"=>[{"_characters"=>["a"]}]}, {"_name"=>"allowed", "value"=>[{"_name"=>"digit"}, {"_characters"=>["a"]}]}]'],
  ["required:         digit           ;                        required: []-];", DEFAULT_RULES],
  ["required:         digit           ;                        required: [--];", '[{"_name"=>"required", "value"=>[{"_name"=>"digit"}]}, {"_name"=>"required", "value"=>[{"_characters"=>["-"]}]}, {"_name"=>"allowed", "value"=>[{"_name"=>"digit"}, {"_characters"=>["-"]}]}]'],
  ["required:         digit           ;                        required: [-]];", '[{"_name"=>"required", "value"=>[{"_name"=>"digit"}]}, {"_name"=>"required", "value"=>[{"_characters"=>["-", "]"]}]}, {"_name"=>"allowed", "value"=>[{"_name"=>"digit"}, {"_characters"=>["-", "]"]}]}]'],
  ["required:         digit           ;                        required: [-a--------];", '[{"_name"=>"required", "value"=>[{"_name"=>"digit"}]}, {"_name"=>"required", "value"=>[{"_characters"=>["a", "-"]}]}, {"_name"=>"allowed", "value"=>[{"_name"=>"digit"}, {"_characters"=>["a", "-"]}]}]'],
  ["required:         digit           ;                        required: [-a--------] ];", DEFAULT_RULES],

  # Canonicalization
  ["required: [abcdefghijklmnopqrstuvwxyz]", '[{"_name"=>"required", "value"=>[{"_name"=>"lower"}]}, {"_name"=>"allowed", "value"=>[{"_name"=>"lower"}]}]'],
  ["required: [abcdefghijklmnopqrstuvwxy]", '[{"_name"=>"required", "value"=>[{"_characters"=>["a", "b", "c", "d", "e", "f", "g", "h", "i", "j", "k", "l", "m", "n", "o", "p", "q", "r", "s", "t", "u", "v", "w", "x", "y"]}]}, {"_name"=>"allowed", "value"=>[{"_characters"=>["a", "b", "c", "d", "e", "f", "g", "h", "i", "j", "k", "l", "m", "n", "o", "p", "q", "r", "s", "t", "u", "v", "w", "x", "y"]}]}]']
]

number_of_passes = 0
number_of_failures = 0
total_tests = 0
input_and_expected_results.each do |pair|
  total_tests += 1

  input_string, expected_result = pair
  puts "   Input: #{input_string}"
  result = (javaScriptContext.call "parsePasswordRules", input_string).to_s
  puts "  Result: #{result}"
  puts "Expected: #{expected_result}"

  did_pass = result == expected_result
  if did_pass
    puts "PASS"
    number_of_passes += 1
  else
    puts "FAIL"
    number_of_failures += 1
  end
  puts ""
end

puts "#{number_of_passes} of #{total_tests} passed."

[rmondello]

@prabhu I’ve repurposed this issue to be solely about unit tests. Can you report new issues for the {} and comments issues?

[igor-makarov]

@rmondello I was actually thinking of setting up a GitHub action to enforce alphabetic order, would you be interested in that?

You could hook the tests up to it later.

[rmondello]

@igor-makarov I don’t know if you have permission to do that, but I’d like to see you try!

[rmondello]

Another test we should have: that all passwordRules parse properly using the parser.

[igor-makarov]

@rmondello could even test the password change URLs for redundancy!

[billjive]

• Single line if conditions are lacking braces. Did we forget goto fail?

This is a holdover from the WebKit project’s style. I’d agree with you that we should deviate from it here. If you’d like to send in a PR for that, I’d happily review it. Otherwise, I’ll get around to that sometime!

I opened 201 to address this!

[billjive]

• no unit tests

I have a test suite written for the parser, but I haven’t figured out how to make it or something like it run on Github yet. I’ll inline it at the end of this comment, and maybe you’ll have ideas on how we can integrate it into the project. It’s a Ruby script that has a dependency on execjs.

I've looked in to this and your (@rmondello) Ruby tests can be converted to javascript using the mocha framework I created (see 168). However, the way PasswordRulesParser.js makes it hard to test. Normally it would be written as a module with an exported function:

// In PasswordRulesParser.js
let parser = {}

// ... existing code here until the last method ...

// WAS: function parsePasswordRules(input, formatRulesForMinifiedVersion)
parser.parsePasswordRules = function(input, formatRulesForMinifiedVersion)

module.exports = parsePasswordRules

Once you have that then your test class looks like this:

var assert = require('assert')
var parser = require('../../tools/PasswordRulesParser.js')

describe('something', function () {
    it('should work', function() {
        var parsed = parser.parsePasswordRules("minlength: 6;", true)
        assert.equal(parsed.length, 1, "One rule should have been found")
    })
})

This is better however parsePasswordRules returns an array of password rules which are classes defined in PasswordRulesParser.js. So, ideally those would be exported as well.

So my question is, would you support these kinds of changes to the code?

There is an alternative: you can use something like rewire in the test class which essentially introspects the code and provides ways to extract what you need. It'd be a way to make zero changes to PasswordRulesParser.js (not even make it a module) and still make progress on a big test suite.

Quick version using rewire:

var assert = require('assert')
var rewire = require('rewire')

var parser = rewire('../../tools/PasswordRulesParser.js')

describe('something', function () {
    it('should work', function() {
        parsePasswordRules = parser.__get__('parsePasswordRules')
        var parsed = parsePasswordRules("minlength: 6;", true)
        assert.equal(parsed.length, 1, "One rule should have been found")
    })
})

[billjive]

I created a quick version of this, you can see it in my fork.

[billjive]

@rmondello I thought I'd post an update here rather than starting a new issue.

I have a couple of examples of finished test that convert your Ruby suite to Javascript.

Based on what I wrote above there are two options: 1) use the objects you've created (example: CustomCharacterClass) as part of the test suite, or 2) more directly mimic the Ruby tests and make comparisons with JSON. It's really chef's choice and comes down to how you want to be able to read/write the tests.

• Example using objects
• Example using JSON

I guess I'd prefer the JSON -- it's easier to read and requires less introspection of your javascript code (done via a handy library called rewire). The only challenge with the JSON way is to keep the formatting tidy so it's readable.

I'm happy to rebase & make a pull request but I'd love to get your (@rmondello) input before proceeding. Let me know what you think!

[billjive]

@rmondello or anyone else -- any interest in this?