Make WordPress Core

Opened 2 years ago

Closed 22 months ago

Last modified 22 months ago

#56266 closed task (blessed) (fixed)

Backport missing Gutenberg tests to Core

Reported by: antonvlasenko's profile antonvlasenko Owned by: hellofromtonya's profile hellofromTonya
Milestone: 6.1 Priority: normal
Severity: minor Version: 5.9
Component: Editor Keywords: has-patch has-unit-tests commit
Focuses: Cc:

Description

WP_Navigation_Test and WP_Global_Styles_Test test classes haven't been backported from Gutenberg to Core.

Change History (17)

#1 @antonvlasenko
2 years ago

I'm working on a patch for this issue.

#2 @SergeyBiryukov
2 years ago

  • Milestone changed from Awaiting Review to 6.1

This ticket was mentioned in PR #3005 on WordPress/wordpress-develop by anton-vlasenko.


2 years ago
#3

  • Keywords has-patch has-unit-tests added; needs-patch removed

This PR aims to backport WP_Navigation_Test and WP_Global_Styles_Test test classes to Core.
These tests haven't been back ported to Core.

Trac ticket: https://core.trac.wordpress.org/ticket/56266

#4 @antonvlasenko
2 years ago

Testing instructions for https://github.com/WordPress/wordpress-develop/pull/3005:
Make sure that the unit tests pass.
That PR only contains unit tests.
Props to @ntsekouras for discovering this issue.

Original tests classes:

  1. https://github.com/WordPress/gutenberg/blob/release/13.6/phpunit/global-styles-test.php
  2. https://github.com/WordPress/gutenberg/blob/release/13.6/phpunit/navigation-test.php
Last edited 2 years ago by antonvlasenko (previous) (diff)

#5 @costdev
2 years ago

  • Keywords commit added

Thanks @antonvlasenko! I approved the PR and marking for commit consideration. @SergeyBiryukov Do you have time to take a look at this one?

ironprogrammer commented on PR #3005:


2 years ago
#6

Each of these migrated tests have the same test method name, test_it_correctly_handles_different_post_types(), which seems awkward compared with others in the project. Maybe it's just me, but an alternative might be to simplify it to test_handle_different_post_types(). Why?

  • The inclusion of _it_ seems unnecessary in the context of these isolated tests, *UNLESS* additional clarity is desired, in which case this should include the function name under test, which seems pretty common.
  • We should assume that unit tests already test for "correct" results, making that word redundant. So "incorrect"/error messages for assertions should be defined in their message parameters.
  • Like with git commit messages, the imperative voice tends to read more easily.

Likewise, `test_wp_filter_global_styles_post_returns_correct_value()` mentions "correct" in its name, but the test is to "return the same value", hence _return_same_value sounds closer to the intent.

Of course none of this is critical, and maybe only matters to those who stare at unit tests and terminal output! Just </two-cents> 😂

costdev commented on PR #3005:


2 years ago
#7

@ironprogrammer Each of these migrated tests have the same test method name, test_it_correctly_handles_different_post_types(), which seems awkward compared with others in the project. Maybe it's just me, but an alternative might be to simplify it to test_handle_different_post_types().

I think test_should_ / test_should_not_ have precedent as test method prefixes in Core and are certainly consistent with wider unit/integration/e2e testing, making it easier for experienced testers when they begin contributing to Core.

I had a longer message in mind, but as this comment will appear on the Trac ticket, I've instead put this in a Gist to keep the noise down. I'm curious to hear your thoughts on this, so feel free to DM me on Slack to discuss further.

anton-vlasenko commented on PR #3005:


2 years ago
#8

There is no need to prefix the data provider with data_test_.

My bad. Thanks for the review, @costdev!
I've fixed it.

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


22 months ago

robinwpdeveloper commented on PR #3005:


22 months ago
#10

PR looks good to me! 👍

#11 @robinwpdeveloper
22 months ago

Latest changes on the PR looks good to me.

#12 @audrasjb
22 months ago

  • Type changed from enhancement to task (blessed)

Let's convert this to a task instead of an enhancement.

#13 @hellofromTonya
22 months ago

  • Owner set to hellofromTonya
  • Status changed from new to reviewing

Self-assigning for review and commit.

hellofromtonya commented on PR #3005:


22 months ago
#14

Rebased on top of current trunk to ensure the new tests cover any changes without introducing any test leaks to other tests.

#15 @hellofromTonya
22 months ago

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

In 54382:

Editor: Add PHPUnit tests for 5.9.0 new functions.

During the 5.9.0 cycle, tests were missed during the porting from Gutenberg to Core for the following functions:

  • _disable_block_editor_for_navigation_post_type()
  • _disable_content_editor_for_navigation_post_type()
  • _enable_content_editor_for_navigation_post_type()
  • wp_filter_global_styles_post()

This commit adds new test classes for these functions.

Reference:

Follow-up to [52298], [52145], [52052].

Props antonvlasenko, costdev, ironprogrammer, robinwpdeveloper, hellofromTonya.
Fixes #56266.

#16 @hellofromTonya
22 months ago

  • Version changed from trunk to 5.9

hellofromtonya commented on PR #3005:


22 months ago
#17

Committed via changeset https://core.trac.wordpress.org/changeset/54382. Thank you everyone for your contributions!

Note: See TracTickets for help on using tickets.