Make WordPress Core

Opened 3 months ago

Closed 5 weeks ago

#61238 closed defect (bug) (fixed)

Tests: Ticket annotations should not include any trailing characters

Reported by: jonsurrell's profile jonsurrell Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 6.6 Priority: normal
Severity: normal Version:
Component: Build/Test Tools Keywords: has-patch has-unit-tests
Focuses: Cc:

Description

PHPUnit tests use the @ticket 1234 annotation to reference tickets, but are also used as a PHPUnit group so that tests can be filtered like this:

phpunit --group 1234

Where only tests with the @ticket 1234 will run (or a 1234 group).

Some annotations include additional characters, such as a full stop "." or other details on the ticket line. This breaks the group-based ticket filtering.

Some changes would improve things:

  • Trailing characters should be removed from the ticket numbers.
  • The ticket to group matching should trim the ticket number from the annotation to match only the initial digits (something like /^\d+/).
  • A PHP code sniff could prevent adding extra characters to the ticket number.

Change History (12)

#1 in reply to: ↑ description @jonsurrell
3 months ago

Tests with tickets that will not currently match --group filtering as expected:

grep -ER '@ticket\s+[0-9]+[^0-9]' tests/phpunit/tests
tests/phpunit/tests/privacy/wpPrivacyGeneratePersonalDataExportGroupHtml.php:    * @ticket 46895 Updated to remove </h2> from test to avoid Count introducing failure.
tests/phpunit/tests/cron.php:    * @ticket 45976.
tests/phpunit/tests/cron.php:    * @ticket 45976.
tests/phpunit/tests/cron.php:    * @ticket 45976.
tests/phpunit/tests/db.php:                      * @ticket 56933.
tests/phpunit/tests/db.php:                      * @ticket 52506.
tests/phpunit/tests/formatting/date.php:                        // @ticket 56468.

I'll submit a patch for this.

Last edited 3 months ago by jonsurrell (previous) (diff)

#2 @swissspidy
3 months ago

  • Milestone changed from Awaiting Review to 6.6

This ticket was mentioned in PR #6564 on WordPress/wordpress-develop by @jonsurrell.


3 months ago
#3

  • Keywords has-patch has-unit-tests added

@jonsurrell commented on PR #6564:


3 months ago
#4

There's one ticket annotation in an inline comment in a data provider from https://core.trac.wordpress.org/ticket/56468:

(Line 243 here)

https://github.com/WordPress/wordpress-develop/blob/9614cf53784a8fd4391e4ac5e4156664e4264e7e/tests/phpunit/tests/formatting/date.php#L228-L249

I'm not quite sure what to do with that one.

#6 @SergeyBiryukov
3 months ago

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

#7 @SergeyBiryukov
3 months ago

In 58164:

Tests: Remove trailing characters from @ticket annotations.

Follow-up to [44693], [46209], [54230], [54733], [55151].

Props jonsurrell.
See #61238.

@SergeyBiryukov commented on PR #6564:


3 months ago
#8

Thanks for the PR! Merged in r58164.

#9 @jonsurrell
3 months ago

Does anyone know where the @ticket annotations are processed? I found this function, but my changes to that file didn't seem to have any impact:

https://github.com/WordPress/wordpress-develop/blob/236a7e0fbf12e650f109d62eacd7d90b47832847/tests/phpunit/includes/phpunit6/compat.php#L21-L39

I'd like to see if we could implement this idea:

The ticket to group matching should trim the ticket number from the annotation to match only the initial digits (something like /^\d+/).

Then this would be less likely to break in the future and tickets could include notes:

/**
 * @ticket 123 Make sure the thing isn't broken.
 */
Last edited 3 months ago by jonsurrell (previous) (diff)

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


3 months ago

#11 @jonsurrell
3 months ago

`@ticket` is a PHPUnit annotation. I thought it was something special.

It doesn't look like something to change at the annotation-handling level.

This seems like something that could have a sniff. I think this ticket can be closed.

#12 @SergeyBiryukov
5 weeks ago

  • Resolution set to fixed
  • Status changed from reviewing to closed
Note: See TracTickets for help on using tickets.