Make WordPress Core

Opened 8 years ago

Closed 8 years ago

#35199 closed defect (bug) (fixed)

WP_UnitTest_Generator_Sequence is generating incorrect sequences

Reported by: oddoneout's profile OddOneOut Owned by: boonebgorges's profile boonebgorges
Milestone: 4.6 Priority: normal
Severity: normal Version: 4.4
Component: Build/Test Tools Keywords: has-patch commit
Focuses: Cc:

Description (last modified by johnbillion)

As done in [35244], the change to use a static incrementer seems to cause the sequence class to generate incorrect sequences. For example when I create two posts, this is the result:

Post 1

Post title 0
Post content 1
Post excerpt 2

Post 2

Post title 1
Post content 2
Post excerpt 3

Shouldn't it be like this:

Post 1

Post title 1
Post content 1
Post excerpt 1

Post 2

Post title 2
Post content 2
Post excerpt 2

Also, is there a way to reset the count? Previously the factory can be reset in setUp but now it's impossible to do so.

Attachments (1)

35199.diff (2.2 KB) - added by boonebgorges 8 years ago.

Download all attachments as: .zip

Change History (17)

#1 @johnbillion
8 years ago

  • Description modified (diff)

#2 @boonebgorges
8 years ago

  • Keywords reporter-feedback added

@OddOneOut Thanks for the ticket!

The central purpose of WP_UnitTest_Generator_Sequence is to ensure uniqueness for things like slugs and user names. The static incrementor enforces uniqueness quite nicely, though you're correct that it increments quite aggressively.

Are there practical problems that arise because of the current behavior? I assume you had previously written tests that assume that the incrementor in (say) the post_name will match the incrementor in the post_title? I am sympathetic to this. But at the same time, if you plan to make assertions about specific properties of a fixture, you probably ought to be specifying these properties when setting up the fixture.

#3 follow-up: @OddOneOut
8 years ago

  • Keywords reporter-feedback removed

@boonebgorges thanks for your quick reply.

Let's say I have a function in my plugin that lists all post titles. If I create a test for it, naturally I will create posts using the factory before every test (each test has a different number of posts), so that:

  • Test 1 will assert this output: "Post title list: Post title 1, Post title 2"
  • Test 2 will assert this output: "Post title list: Post title 1, Post title 2, Post title 3"

Which means for every test the count must be reset.

Replying to boonebgorges:

The static incrementor enforces uniqueness quite nicely, though you're correct that it increments quite aggressively.

IMO the current implementation is incorrect, unless it is intended (which does not make sense to me).

I might be wrong but it seems that in 4.4 the fixtures are expected to be setup once in the static wpSetUpBeforeClass function. If really so, this is a very non-BC change. I understand that 4.4 doesn't need to be backward-compatible with 4.3 in every way, but I don't really understand the reasons, as we should be able to setup shared fixtures as well as test-specific fixtures.

I know that this whole Build/Test tool is designed specifically for the WordPress core, and using it for testing plugins/themes is just a "hack". But it would be nice to, and in fact there are many developers that are already relying on this tool.

Anyway, if there's a workaround for my particular use case, please advise. Thank you.

Last edited 8 years ago by OddOneOut (previous) (diff)

#4 in reply to: ↑ 3 ; follow-up: @boonebgorges
8 years ago

  • Milestone changed from Awaiting Review to 4.5

Replying to OddOneOut:

@boonebgorges thanks for your quick reply.

Let's say I have a function in my plugin that lists all post titles. If I create a test for it, naturally I will create posts using the factory before every test (each test has a different number of posts), so that:

  • Test 1 will assert this output: "Post title list: Post title 1, Post title 2"
  • Test 2 will assert this output: "Post title list: Post title 1, Post title 2, Post title 3"

Which means for every test the count must be reset.

It only "must be reset" if you have written tests that assume that the incrementor will be reset between tests :)

All this being said, I think it's worth considering the following:

  1. a method that test methods can use to reset the incrementor.
  2. more consistent use of incrementors within methods (so that a post with "Post title 2" will also use the incrementor 2 in all of its other properties)

I do *not* think core tests need to reset the incrementor between tests; doing so will make our tests more fragile by causing insert failures when a prior test fails to clean up after itself.

#5 in reply to: ↑ 4 @OddOneOut
8 years ago

Replying to boonebgorges:

All this being said, I think it's worth considering the following:

  1. a method that test methods can use to reset the incrementor.
  2. more consistent use of incrementors within methods (so that a post with "Post title 2" will also use the incrementor 2 in all of its other properties)

Or maybe a way to disable the static incrementor.

I do *not* think core tests need to reset the incrementor between tests; doing so will make our tests more fragile by causing insert failures when a prior test fails to clean up after itself.

I did not say so, what I mean is that right now there's no way to setup test-specific fixtures with the factory, and I believe there are use cases for it.

Thanks for consideration, hopefully there are patches for 4.5, right now I will just revert to 4.3 then.

#6 @wonderboymusic
8 years ago

if we're setting up fixtures statically, there's almost no way ensure uniqueness across tests without having the static counter in WP_UnitTest_Generator_Sequence. WP_UnitTest_Generator_Sequence should be completely agnostic to when it is called and where. Things like post titles and slugs should also remain dynamic in tests. So, instead of asserting against magic numbers and strings, you can assert against a string that is built with $post1->post_title, etc.

#7 @jorbin
8 years ago

  • Keywords close added

I am in agreement with @wonderboymusic on this. I think that we can close this as invalid with the suggestion to never rely on any sort of magic numbers or strings in tests.

#8 @boonebgorges
8 years ago

  • Keywords close removed
  • Milestone 4.5 deleted
  • Resolution set to invalid
  • Status changed from new to closed

#9 follow-up: @OddOneOut
8 years ago

What about the current invalid behaviour i.e. "Post title 0, Post content 1, Post excerpt 2" (which should be "Post title 1, Post content 1, Post excerpt 1")?

Also, the issue I'm mentioning here is not whether we should use $post1->post_title or just "Post title 1" to test, but about not relying on the global state to test. At least there should be a way to reset the factory or disable the static incrementor (as I've said above).

Hopefully you guys will reconsider this.

#10 in reply to: ↑ 9 @boonebgorges
8 years ago

Replying to OddOneOut:

What about the current invalid behaviour i.e. "Post title 0, Post content 1, Post excerpt 2" (which should be "Post title 1, Post content 1, Post excerpt 1")?

The behavior is only "invalid" if you write tests that expect the incrementor in these fields to be the same for the same post. The suggestion in this thread is that you do not write tests with this expectation, but instead either (a) use the dynamically generated values for your assertions, or (b) feed specific values into the factory method when building the features.

That said, if you can provide a patch or suggest a strategy that will enforce this behavior - using the same incrementor for a given feature - without rolling back the general improvement of persistent incrementors throughout the test run, I don't think there's any harm in including it.

Also, the issue I'm mentioning here is not whether we should use $post1->post_title or just "Post title 1" to test, but about not relying on the global state to test. At least there should be a way to reset the factory or disable the static incrementor (as I've said above).

If "not relying on the global state to test" means "start incrementors over with each test", then that would mean reintroducing the behavior that [35244] was meant to circumvent. Maybe it wasn't fully explained in that commit message or ticket, but non-persistent incrementors cause significant problems in the context of WP's janky automated test suite. For example, generating a user fixture will create a user with an email address that uses the incrementor. If the test fails to clean up after itself properly, a future test will fail due to users not being inserted correctly. This has caused hours of painful debugging for me personally in the past.

I think that if you need a reset method, it indicates that your tests are doing something unnecessarily fragile. That being said, if you'd like to provide a patch for such a method, feel free to post it here and reopen the ticket. Thanks!

#11 @boonebgorges
8 years ago

  • Milestone set to 4.6
  • Resolution invalid deleted
  • Status changed from closed to reopened

As pointed out by @swissspidy, the new WP_UnitTest_Generator_Sequence iterator strategy can cause odd problems when running sets of tests that run in separate threads, such as AJAX tests. In these cases, the $incr value can be incremented in ways that are difficult to predict, resulting in user insert errors and other weirdness.

I think it's possible to fix the separate-thread issue as well as some of the complaints in the current ticket by altering the incrementor routine so that the same incrementor is enforced for a single object. See 35199.diff. Backward compatibility should be maintained for anyone using the back() method, though we won't be using it anymore.

@boonebgorges
8 years ago

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


8 years ago

#13 @swissspidy
8 years ago

  • Milestone changed from 4.6 to 4.5.1

#14 @swissspidy
8 years ago

  • Keywords has-patch commit added

#15 @boonebgorges
8 years ago

  • Milestone changed from 4.5.1 to 4.6

I think we should be able to fix #36578 for 4.5.1 without sneaking this ticket into a minor release. See https://core.trac.wordpress.org/ticket/36578#comment:13.

#16 @boonebgorges
8 years ago

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

In 37299:

Tests: Use the same incrementor for all fields belonging to a given text fixture.

[35244] changed the way that WP_UnitTest_Generator_Sequence() created an
incrementor for object fields (like 'post_name' and 'user_email'), by making
incrementor static across the entire run of the test suite. While this helped
to enforce uniqueness across the tests, it has the side effect of bumping the
incrementor between fields on the same object (so that, eg, the same post might
have post_name "post-12" but post_title "Post 13". By switching to a
technique that uses the same incrementor for each field belonging to a given
fixture, we conform better to the expectations of developers using
WP_UnitTest_Factory.

Fixes #35199.

Note: See TracTickets for help on using tickets.