Comment Template Unit Test: Cover odd/even classes

[ockham]

What?

Add a unit test to cover the Comment Template block's even/odd behavior (i.e. add coverage for #40455).

Why?

Cover against regressions of #40455.

How?

By changing our nested test comments in a way that allows us to detect if odd/even classes are applied correctly.

Testing Instructions

• Verify that PHP unit tests are green in CI.
• Locally, with this branch checked out, revert caf7b7d, rebuild GB, and run npm run test-unit-php. The test should fail now. Here's proof from a previous CI run.

[ockham]

I've tried producing a minimal test case that would fail (this PR doesn't have the fix from #40455 yet), but I'm struggling to find one.

@michalczaplinski Based on your work on that PR, can you help me find one? Currently, I'm using

└─ comment 1
   └─ comment 2
      └─ comment 4
   └─ comment 3

Which I thought would give even-even-odd-odd (since it'd start assigning classes at comment 4 -> 2 -> 3 -> 1), but it didn't. I also tried a few different other variants (see commit log), but to no avail (odd and even always ended up correctly).

You might have a better insight in how this works, can you maybe suggest a different comment hierarchy to test with? 😅

[michalczaplinski]

Hey @ockham !

I think the issue you hit is that the odd and even classes are not applied based on whether the comment_id is an odd or even number! Yeah, I know, that's confusing. I was confused by that as well 😅

Instead, "odd" and "even" refers to the index of the comment, when traversing them depth-first. At least that's how the classes are applied in core so I've used the same logic in the block.

2022-04-20_19-21-07.mp4

This means that your test should be passing (as it indeed is!).

To be honest, I don't know if those classes are particularly useful but I didn't want to deviate from the way they're applied in core.

[ockham]

Hey @michalczaplinski, thanks for the explanation!

I should've been a bit clearer: The motivation for this PR was to create a unit test that would fail if #40455 wasn't applied. While we already had a test with some nested comments, the way they were nested didn't cause the unit test too fail with regards to even/odd classes.

To that end, I actually created this branch from trunk before I merged #40455 (I didn't make this clear in the PR desc but only in one parenthetical sentence in this comment -- my bad). I was then trying to come up with a more complex nesting that would indeed cause tests to fail -- something that would more closely resemble the layout from your screenshots in #40455 (but wasn't quite as complex, to keep the test simple enough to reason about). However, even after some attempts (see the PR's commit history 😅 ), I didn't manage to produce a simple-enough nesting that would cause the wrong even/odd classes. (I also have a problem running PHP unit tests locally, something seems broken there: whatever I change, it's always the same set of -- unrelated -- tests that fail. So I had to keep pushing commits to this PR to rely on CI instead 😕 )

Anyway, we can keep trying for a bit longer. What we shouldn't do is merge this PR as-is; it has no added value compared to the previous nesting layout, since the tests would pass in both cases. Instead, we need to make it fail (with regard to even/odd), and verify that when rebased on trunk, it passes.

[ockham]

Weirdly enough, if I recreate the PR's current nesting in the block editor, I do get the wrong even/odd classes, as desired 🤔 😕

[michalczaplinski]

@ockham I've checked out the branch and I've ran the tests and the output is what I would expect 🤔 Am I missing something here?

[ockham]

Oh wow, that's super interesting. Looks like the tests are failing exactly as they should when you run them locally @michalczaplinski 😮

Well the weirdest thing to me is that in CI, they pass:

And it doesn't seem like the test is being ignored, since they did fail earlier when I had made other mistakes in the expected block markup 😕

I'm a bit stumped here -- the whole point of this exercise would be for the tests to alert us if we mess up the comment classes logic via CI -- i.e. to block a PR from merging...

[michalczaplinski]

I think this is because github actions do not care that you've created this branch before merging #40455 😄 The action is run on the repository in the state that it would have after the current change in the PR were applied to trunk.

You can verify this by running locally the same git commands as the github action:

1. git -c protocol.version=2 fetch --no-tags --prune --progress --no-recurse-submodules --depth=1 origin +becf27eacf42b2f0ea673901fc4cb212b717bad7:refs/remotes/pull/40471/merge which is run here

2. git checkout --progress --force refs/remotes/pull/40471/merge which is run here

If you open the Comment Template's index.php, you can see that it includes the changes from #40455 🙂

So, I think that we can merge this PR as is, as it should indeed fail if there is a regression in the way the CSS classes are applied.

[ockham]

I think this is because github actions do not care that you've created this branch before merging #40455 😄 The action is run on the repository in the state that it would have after the current change in the PR were applied to trunk.

I gotta admit, I thought that GHA worked differently than that 🤔 😅

Anyway, good point! I'd like to put it to a test before merging: I'll rebase on trunk, and I'll revert caf7b7d. That should make it fail 😬

[ockham]

I think this is because github actions do not care that you've created this branch before merging #40455 😄 The action is run on the repository in the state that it would have after the current change in the PR were applied to trunk.

I gotta admit, I thought that GHA worked differently than that 🤔 😅

Anyway, good point! I'd like to put it to a test before merging: I'll rebase on trunk, and I'll revert caf7b7d. That should make it fail 😬

Yes! It did!

[ockham]

PR only increases unit test coverage; change has been shown to cover a previous regression, and to otherwise pass reliably (see). Going to merge.

[ockham]

Backported to Core (thanks to @DAreRodz) in this PR: WordPress/wordpress-develop#2649