Fix false-positive detection of Labels

[h3xx]

Add regression tests for them too. Update change log.

See fabe204067f01f9e93d9c6be762b5019d2a7b7c4 for examples.

Closes #262

[oalders]

Thanks @h3xx! I'm going to close and re-open so that I can kick off a CI build.

[oalders]

We've got a test failure in the build.

[h3xx]

I think I fixed the test.

Man, I bet the test plan count gets annoying to update every single time tests are touched. Quietly questioning whether it would be wiser to just remove the test count altogether from each of these extensive tests.

Also rebased off current 'master' branch.

[h3xx]

Can you please run the CI build again?

[oalders]

Quietly questioning whether it would be wiser to just remove the test count altogether from each of these extensive tests.

I'm not a fan of test counts, but I will defer to @wchristian on that one.

[oalders]

Can you please run the CI build again?

Running now

[oalders]

This all looks good to me. I suspect that @wchristian would want the regression tests in the first commit added as TODO items and then have the TODO removed in the commit which introduces the fix. This would ensure that git bisect doesn't get broken by this PR.

[haarg]

The test counts are rather silly. No reason to remove them in this PR assuming it doesn't need any other changes, but if it does, replacing them with done_testing sounds like a good idea to me.

[h3xx]

I suspect that @wchristian would want the regression tests in the first commit added as TODO items and then have the TODO removed in the commit which introduces the fix. This would ensure that git bisect doesn't get broken by this PR.

Updated.

Since I couldn't figure out how to mark a regression test code+dump file couplet as TODO without marking the entire series of regression tests, I simply reversed the order of the commits. It applies the fix first, THEN adds tests. This ensures tests will pass for any given point in the repo history. :-)

[oalders]

@wchristian any concerns with this PR? It's essentially a 3 line change. I realize there may be more to consider, but the diff is not a heavy review. :)

[h3xx]

Updated: Fixed PR reference in changelog.

[wchristian]

Aight, my day is super hectic, but a quick explanation:

I ask for things to be committed as first, broken todo tests, and then a fix that makes those tests pass AND marks them as not todo, so i can, when trying to merge multiple separate changes, combine the tests first, and then try out various combinations of fix commits to see which specific constellations break things; because often different fixes work on their own, but in combination break something new. I even have some code to detect if something makes something pass, when it should be broken and todo.

This can be useful even after the fact if i realize that a later fix conflicts with another previous fix and i create research branches to combine things in different ways.

Thus, without having looked at the code, maybe you can have the two test scripts check for the filename of the dumps and mark their subtest blocks as todo in entirety for those specific dumps?

Sorry for the hassle if it is too much, but i hope my logic makes sense. :)

[wchristian]

oh yeah, and the changelog change, please amend that into the fix commit :D

[h3xx]

Okay, I think I understand. I updated my commits to:

1. Add my new tests under t/data/08_regression_todo, altering 08_regression.t to scan that directory
2. Fix the issue
3. Move my tests to t/data/08_regression and remove references to the t/data/08_regression_todo directory.

I mimicked what I thought I saw passing in and out of t/07_token.t in the commit history. I think I even got the test count correct at all commits.

No change to the actual code at the head of my branch, just in the steps to getting there.

[wchristian]

Looks good to me, i'll be making a release with it tomorrow. I will in doing so also be squashing the latter two commits together, since the fix and un-todoing should be in the same commit, but if you wanna do it before i get to it, that'd be fine too. :)

[wchristian]

thanks a bunch!

merged and released as PPI-1.277

let's see what this release breaks :D

[wchristian]

Turns out this did indeed break something because Perl::Critic expected PPI to be broken. The relevant PR is here: https://github.com/Perl-Critic/Perl-Critic/pull/1048

[h3xx]

Relevant XKCD: https://xkcd.com/1172

[wchristian]

Haha, very appropriate. Sadly i didn't think of doing a dependent test until after the release. :)