Make WordPress Core

Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#32688 closed task (blessed) (fixed)

Customizer Menus: add QUnit tests

Reported by: designsimply's profile designsimply Owned by: westonruter's profile westonruter
Milestone: 4.3 Priority: high
Severity: normal Version: 4.3
Component: Customize Keywords: has-patch commit
Focuses: Cc:

Description

The Customizer Menus feature needs QUnit tests.

Moved from https://github.com/voldemortensen/menu-customizer/issues/68

Attachments (8)

32688.diff (5.0 KB) - added by jorbin 9 years ago.
32688.2.diff (2.0 KB) - added by adamsilverstein 9 years ago.
Start adding some tests
32688.3.diff (6.1 KB) - added by adamsilverstein 9 years ago.
svn add new files
32688.4.diff (6.3 KB) - added by adamsilverstein 9 years ago.
add defaults setup test
32688.5.diff (4.3 KB) - added by jorbin 9 years ago.
32688.6.diff (4.3 KB) - added by westonruter 9 years ago.
Additional change: https://github.com/xwp/wordpress-develop/commit/3443f080487dbbfbf5f96e20187f4c2f1abd8bff
32688.7.diff (13.7 KB) - added by westonruter 9 years ago.
Additional changes: https://github.com/xwp/wordpress-develop/compare/3443f08...8ade858
32688.8.diff (32.9 KB) - added by westonruter 9 years ago.
Additional changes: https://github.com/xwp/wordpress-develop/compare/8ade858...3fe53c2a

Download all attachments as: .zip

Change History (28)

#1 @celloexpressions
9 years ago

Who's up for tackling these?

#2 @obenland
9 years ago

  • Owner set to westonruter
  • Status changed from new to assigned

#3 @designsimply
9 years ago

  • Priority changed from normal to high

#4 @valendesigns
9 years ago

  • Version set to trunk

#5 @obenland
9 years ago

Why is there no patch on this yet?

#6 @westonruter
9 years ago

  • Owner westonruter deleted

Help needed.

@jorbin
9 years ago

#7 @jorbin
9 years ago

I'm working on getting some tests here, though it would be really helpful if someone more intimate with this code wrote them. My above patch starts adding the code needed to run tests without actually adding any tests. That's next. Hints, tips and pointers are appreciated.

@adamsilverstein
9 years ago

Start adding some tests

@adamsilverstein
9 years ago

svn add new files

#8 @adamsilverstein
9 years ago

Thanks @jorbin for getting this started. I'm going to start writing some tests!

In 32688.3.diff:

  • Add test to verify that generatePlaceholderAutoIncrementId generates unique IDs
  • Add some more setup steps, include menu customizer, add window.wpNavMenu setup

Replying to jorbin:

I'm working on getting some tests here, though it would be really helpful if someone more intimate with this code wrote them. My above patch starts adding the code needed to run tests without actually adding any tests. That's next. Hints, tips and pointers are appreciated.

@adamsilverstein
9 years ago

add defaults setup test

#9 @jorbin
9 years ago

I am getting this cleaned up so we can get some tests added now. I think the next step needs to be testing some of the api.Menus.MenuItemControl pieces, such as move left, move right, move up, and move down. _changeDepth and _changePosition both throw errors, we should use throws to make sure that they are properly thrown when those error conditions exist.

#10 @jorbin
9 years ago

In 33451:

Add Initial JS Unit Tests for Menu Customizer

While these two tests will help ensure we don't repeat our mistakes, they mostly help lay the foundation for more tests that still need to be written.

See #32688
Props adamsilverstein, jorbin

@jorbin
9 years ago

#11 follow-up: @jorbin
9 years ago

Above patch continues to spec out the fixture and adds an additional test, however it also causes many of the existing panel and settings tests to fail.

This ticket was mentioned in Slack in #core by mark. View the logs.


9 years ago

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

Replying to jorbin:

Above patch continues to spec out the fixture and adds an additional test, however it also causes many of the existing panel and settings tests to fail.

Trivial fix for this in 32688.6.diff: we just have to extend the window._wpCustomizeSettings.sections object instead of overriding it.

PR with all patches: https://github.com/xwp/wordpress-develop/pull/107

#14 @westonruter
9 years ago

In 32688.7.diff: flesh out fixtures for nav menus and add test for non-empty menus, add todos for tests, remove setup/teardown resets for now.

Last edited 9 years ago by westonruter (previous) (diff)

#15 @westonruter
9 years ago

  • Keywords has-patch added; needs-patch removed

Well, I feel like 32688.8.diff may be as far as we can take QUnit tests for Menu Customizer in 4.3. Beside the limited time left, the Menu Customizer functionality really begs for integration/acceptance tests and not pure unit tests (even the PHPUnit tests in Core are often not pure unit tests either). I think what we really need to fully test the Customizer is a testing approach that has a real environment (i.e. the vanilla environment created for initial PHPUnit test state) which allows JS to access data provided by PHP at page load time, *and* via Ajax requests, and evaluate the ability to perform end-to-end workflow operations on menus in the Customizer. This is something I've talked about with @jorbin to start writing Selenium scripts to run through the Customizer in the various acceptance tests.

So I suggest that the patch here be committed and we close this out to then turn focus in JS testing to writing acceptance tests.

#16 @obenland
9 years ago

@ocean90, @markjaquith, can @westonruter get the go-ahead here? It would be nice to have that fixed with RC2.

#17 @markjaquith
9 years ago

Yeah, go for it. Passing tests should always be good to go, as they don't affect /build/.

#18 @jorbin
9 years ago

  • Keywords commit added

Patch looks good to me.

#19 @westonruter
9 years ago

  • Owner set to westonruter
  • Resolution set to fixed
  • Status changed from assigned to closed

In 33528:

Customizer: Add QUnit tests for menus.

Props adamsilverstein, westonruter, jorbin.
Fixes #32688.

#20 @iseulde
9 years ago

In 33562:

JSHint for [33528]: remove logging

See #32688.

Note: See TracTickets for help on using tickets.