WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#25088 closed task (blessed) (fixed)

password-strength-meter.js should have unit tests

Reported by: jorbin Owned by: 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 5 years ago.
25088.diff (68.3 KB) - added by jorbin 5 years ago.
25088.2.diff (68.3 KB) - added by jorbin 5 years ago.
With redundant semicolons and early commas
25088.3.diff (68.5 KB) - added by jorbin 5 years ago.
25088.4.diff (70.5 KB) - added by jorbin 5 years ago.
25088.5.diff (70.8 KB) - added by iandunn 5 years ago.
25088.6.diff (1.1 KB) - added by iandunn 5 years ago.
Adds 'username in password should be penalized' test
25088-spacing.patch (6.1 KB) - added by TobiasBg 5 years ago.
25088-spacing-braces.patch (6.0 KB) - added by TobiasBg 5 years ago.
also changes } ); to });

Download all attachments as: .zip

Change History (43)

#1 @kadamwhite
5 years ago

  • Cc kadamwhite added

#2 @aaroncampbell
5 years ago

  • Cc aaroncampbell added

#3 @WraithKenny
5 years ago

  • Cc Ken@… added

#4 @adamsilverstein
5 years ago

  • Cc adamsilverstein@… added

#5 @brianhogg
5 years ago

  • Cc brian@… added

#6 @jorbin
5 years ago

  • Milestone changed from 3.8 to 3.7

@jorbin
5 years ago

@jorbin
5 years ago

#7 @jorbin
5 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
5 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
5 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
5 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
5 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
5 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
5 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
5 years ago

With redundant semicolons and early commas

#14 @jorbin
5 years ago

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

@jorbin
5 years ago

#15 @jorbin
5 years ago

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

@jorbin
5 years ago

#16 @jorbin
5 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
5 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 -- it's using spaces instead of tabs, missing whitespace around parenthesis, etc -- but I don't have time for that tonight.

Last edited 5 years ago by iandunn (previous) (diff)

@iandunn
5 years ago

#18 @iandunn
5 years ago

  • Cc ian.dunn@… added

#19 @nacin
5 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
5 years ago

In 25166:

Add QUnit to WordPress core for JavaScript unit testing.

props jorbin.
see #25088.

#21 @nacin
5 years ago

In 25167:

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

props jorbin.
see #25088.

#22 follow-up: @iandunn
5 years ago

Was there something wrong with 5.diff?

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

Replying to iandunn:

Was there something wrong with 5.diff?

I didn't notice it. Can you refresh?

@iandunn
5 years ago

Adds 'username in password should be penalized' test

#24 in reply to: ↑ 23 @iandunn
5 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_
5 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
5 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_
5 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
5 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
5 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
5 years ago

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

#31 @nacin
5 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
5 years ago

also changes } ); to });

#32 @TobiasBg
5 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
5 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
5 years ago

In 25294:

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

Note: See TracTickets for help on using tickets.