Make WordPress Core

Opened 11 years ago

Closed 10 years ago

Last modified 10 years ago

#25088 closed task (blessed) (fixed)

password-strength-meter.js should have unit tests

Reported by: jorbin's profile jorbin Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 3.7 Priority: normal
Severity: normal Version:
Component: Unit Tests Keywords:
Focuses: Cc:

Description

As discussed in IRC, our goal is to have unit testable javascript. Thepassword-strength-meter.js file will serve as our initial testbed for testing.

This file was chosen since there will likely be some changes to it in 3.7 and it is small and already largely testable.

Attachments (9)

inital-qunit.diff (67.2 KB) - added by jorbin 11 years ago.
25088.diff (68.3 KB) - added by jorbin 11 years ago.
25088.2.diff (68.3 KB) - added by jorbin 11 years ago.
With redundant semicolons and early commas
25088.3.diff (68.5 KB) - added by jorbin 11 years ago.
25088.4.diff (70.5 KB) - added by jorbin 11 years ago.
25088.5.diff (70.8 KB) - added by iandunn 11 years ago.
25088.6.diff (1.1 KB) - added by iandunn 11 years ago.
Adds 'username in password should be penalized' test
25088-spacing.patch (6.1 KB) - added by TobiasBg 11 years ago.
25088-spacing-braces.patch (6.0 KB) - added by TobiasBg 11 years ago.
also changes } ); to });

Download all attachments as: .zip

Change History (43)

#1 @kadamwhite
11 years ago

  • Cc kadamwhite added

#2 @aaroncampbell
11 years ago

  • Cc aaroncampbell added

#3 @WraithKenny
11 years ago

  • Cc Ken@… added

#4 @adamsilverstein
11 years ago

  • Cc adamsilverstein@… added

#5 @brianhogg
11 years ago

  • Cc brian@… added

#6 @jorbin
11 years ago

  • Milestone changed from 3.8 to 3.7

@jorbin
11 years ago

@jorbin
11 years ago

#7 @jorbin
11 years ago

My patch is up and it contains four tests. Based on nacin's comments during IRC that we would likely be switching to zxcvbn.js for the internals of this function, I didn't test as much as could because what is a "good" password now may not be one with the improved checking.

I suggest using this in combination with the patch up on #25096 to make the testing easier.

#8 follow-up: @adamsilverstein
11 years ago

this works great and is a nice template for writing future tests. thanks! i'm almost afraid to ask, why are you leaving off semicolons for each line?

#9 in reply to: ↑ 8 ; follow-ups: @jorbin
11 years ago

Replying to adamsilverstein:

this works great and is a nice template for writing future tests. thanks! i'm almost afraid to ask, why are you leaving off semicolons for each line?

Semicolons are redundant. http://blog.izs.me/post/2353458699/an-open-letter-to-javascript-leaders-regarding

#10 in reply to: ↑ 9 ; follow-up: @aaroncampbell
11 years ago

Replying to jorbin:

Semicolons are redundant. http://blog.izs.me/post/2353458699/an-open-letter-to-javascript-leaders-regarding

One of the things mentioned in that article was that some linters, while working fine with no semi-colons, have a hard time with the comma first approach. It looks like you use that format, have you had any issues with linting?

#11 in reply to: ↑ 10 ; follow-up: @jorbin
11 years ago

Replying to aaroncampbell:

One of the things mentioned in that article was that some linters, while working fine with no semi-colons, have a hard time with the comma first approach. It looks like you use that format, have you had any issues with linting?

Using jshint with laxcomma set to true it is never a problem.

#12 in reply to: ↑ 11 @aaroncampbell
11 years ago

Replying to jorbin:

Using jshint with laxcomma set to true it is never a problem.

Sweet! I didn't know about the laxcomma option. For anyone interested: http://www.jshint.com/docs/options/#laxbreak

#13 in reply to: ↑ 9 @nacin
11 years ago

Replying to jorbin:

Semicolons are redundant. http://blog.izs.me/post/2353458699/an-open-letter-to-javascript-leaders-regarding

Sure, but please follow existing core style. This also includes commas at the end of statements.

@jorbin
11 years ago

With redundant semicolons and early commas

#14 @jorbin
11 years ago

patch updated to match the styles requested by Mr. Nacin.

@jorbin
11 years ago

#15 @jorbin
11 years ago

I've updated the tests based on the addition of zxcvbn in [25157]

@jorbin
11 years ago

#16 @jorbin
11 years ago

Based on duck_'s suggestion, I added a bunch (34) of test passwords from zxcvbn https://github.com/lowe/zxcvbn/blob/master/test/test.coffee

#17 @iandunn
11 years ago

It'd also be good to have a test to check the user_input field to make sure that any extra words that are passed are penalized. Currently that's broken because of the modifications that were made to zxcvbn, so a unit test would help make sure that doesn't happen again.

I'm uploading a patch with that added. Note that the test currently fails, which is the expected behavior since Jon's latest patch hasn't been committed yet.

password-strength-meter.js also needs some reworking around the JS coding standards (using spaces instead of tabs, missing whitespace around paranthesis, etc), but I don't have time for that tonight.

Version 0, edited 11 years ago by iandunn (next)

@iandunn
11 years ago

#18 @iandunn
11 years ago

  • Cc ian.dunn@… added

#19 @nacin
11 years ago

In 25165:

Move PHPUnit tests into a tests/phpunit directory.

wp-tests-config.php can/should reside in the root of a develop checkout. phpunit should be run from the root.

see #25088.

#20 @nacin
11 years ago

In 25166:

Add QUnit to WordPress core for JavaScript unit testing.

props jorbin.
see #25088.

#21 @nacin
11 years ago

In 25167:

Add QUnit tests for password-strength-meter.js.

props jorbin.
see #25088.

#22 follow-up: @iandunn
11 years ago

Was there something wrong with 5.diff?

#23 in reply to: ↑ 22 ; follow-up: @nacin
11 years ago

Replying to iandunn:

Was there something wrong with 5.diff?

I didn't notice it. Can you refresh?

@iandunn
11 years ago

Adds 'username in password should be penalized' test

#24 in reply to: ↑ 23 @iandunn
11 years ago

Replying to nacin:

Replying to iandunn:

Was there something wrong with 5.diff?

I didn't notice it. Can you refresh?


Done.

#25 follow-up: @duck_
11 years ago

In 25175:

Test that passwords containing the username are penalised.

Tidy up some spelling, indentation and whitespace whilst we're at it.

Props iandunn. See #25088.

#26 in reply to: ↑ 25 ; follow-up: @jorbin
11 years ago

Replying to duck_:

In 25175:

Test that passwords containing the username are penalised.

Tidy up some spelling, indentation and whitespace whilst we're at it.

Props iandunn. See #25088.

Thanks for the spelling corrections :)

I think we can close this since we do now have unit tests and that any new tests for password-strength-meter.js we wish to add can be new tickets.

#27 in reply to: ↑ 26 @duck_
11 years ago

  • Resolution set to fixed
  • Status changed from new to closed

Replying to jorbin:

I think we can close this since we do now have unit tests and that any new tests for password-strength-meter.js we wish to add can be new tickets.

Indeed.

#28 follow-up: @TobiasBg
11 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Just one more clean-up/spacing/typo patch in 25088-spacing.patch.

#29 in reply to: ↑ 28 @iandunn
11 years ago

Replying to TobiasBg:

Just one more clean-up/spacing/typo patch in 25088-spacing.patch.


Nice, I think it's much more readable that way :)

In the "username in password should be penalized" test, though, I think variables that aren't assigned a value are supposed to be on a single line: http://make.wordpress.org/core/handbook/coding-standards/javascript/#the-var-keyword

#30 @TobiasBg
11 years ago

Good call, Ian! Thanks! I refreshed the patch.

#31 @nacin
11 years ago

It seems that jQuery uses });, not } );, and that seems to be very common for jQuery-like code (including our JS), so we should probably continue to do that.

Our (and jQuery) style also appears to be i++, not ++i. There is a space added after a + at the end of the diff, but not before it. (Look for '+.) Looks good otherwise!

@TobiasBg
11 years ago

also changes } ); to });

#32 @TobiasBg
11 years ago

Argh, those +es :-)
Refreshed patch fixes ++i and the '+.

I'm still unsure about the }); to be honest though... That kind of contradicts spaces in brackets.

test( 'whatever', function() { ... });

looks wrong.
However, 25088-spacing-braces.patch will change that, too, whereas 25088-spacing.patch will not.

#33 @SergeyBiryukov
10 years ago

  • Owner set to SergeyBiryukov
  • Resolution set to fixed
  • Status changed from reopened to closed

In 25275:

Whitespace cleanup for password strength meter tests. props TobiasBg. fixes #25088.

#34 @SergeyBiryukov
10 years ago

In 25294:

Use correct paths in multisite.xml. see [25165]. see #25088.

Note: See TracTickets for help on using tickets.