keithamus / tempus

Tempus - Time for a new Date()
http://tempus-js.com
Other
94 stars 9 forks source link

Regexp testing with g modifier #11

Closed nrbrook closed 12 years ago

nrbrook commented 12 years ago

On line 857, inside the first default parser:

strftimeRegExp.test(b)

Where strftimeRegExp is /\%([OE])?([a-z%])/gi

using test() on a regexp that has the g modifier gives different results each time you run it. See here. This causes problems if Tempus is called repeatedly with the same string format - it fails every fourth attempt.

You can remove the g or reset lastIndex before each test.

Good framework. I saw your talk at LNUG and when I came to start using dates in js I looked it up. Would be good to see a complete API reference though.

Cheers

keithamus commented 12 years ago

Thanks very much for spotting this! I'll get this fixed for 0.2.

As for API docs - they are gradually getting filled in, by 0.2 I hope to have complete API docs complete.

keithamus commented 12 years ago

Having a look into this issue, I cannot find a case where the lastIndex effects the testing of date parsers, for these two reasons:

  1. If the test fails, it returns false and sets lastIndex to 0 (http://es5.github.com/#x15.10.6.2 9a)
  2. If the test passes, the parser function runs, which in turn will always run makeReverseRegex, which in turn will run format.replace(strftimeRegExp.... String.prototype.replace, when given a RegExp as 1st arg will update lastIndex, in the case of a global regexp, will always be reset to 0 because a global keeps going until it reaches false (see http://es5.github.com/#x15.10.6.2 9a again).

So to summarise - while lastIndex is not explicitly reset, it does end up getting reset indirectly by success or failure of the RegExp.

If you could provide a failing testcase in QUnit, that proves there is a failing use case then I will definitely fix this, but I cannot find one for now.

nrbrook commented 12 years ago

The latest version in the repo seems to be ok, but if you run this in Safari 5.1.5 you will see it fail. Perhaps it is best to be defensive against this anyway in case things change in the future, and reset the index before testing? Then you aren't relying on it being reset elsewhere in the code.

keithamus commented 12 years ago

Agree with your points - but largely for my own peace of mind I wanted to see some failing code, that I could write tests against, and see those actually failing - this way we get the best defensiveness of all - if any code changes relating to the failing test, we an see right away.

Thanks for your efforts, I'll fix this for the 0.2 release.