fix(WaitForProcessing): Shutdown option WaitForProcessing never entered on subscription close

[nico-kn]

As outlined in #2117, the condition to enter the WaitForProcessing closing behavior of a subscription was checking against an undefined field .isEmpty of the inventory instead of checking for the isEmpty() method of the inventory.

In unit tests this went unnoticed since the field was set accordingly in the class under test.

This implicitly changes the default behavior of the subscription close functionality since the previous PR that introduced this functionality (#2068) changed the default behavior without effect as the condition was never entered during close().

---

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

• Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
• Ensure the tests and linter pass
• Code coverage does not decrease (if any source code was changed)
• Appropriate docs were updated (if necessary)

Fixes #2117 🦕

[google-cla]

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

[nico-kn]

@feywind would be great if you could review this change. Linting and tests pass locally.

However, the original WaitForProcessing feature was released as default behavior and this behavior change came with a minor version increment to 5.2.0. I would propose to release this as 6.0.0 or 5.3.0 instead of 5.2.1 since the default behavior change is a runtime relevant change for services consuming this client library.

[feywind]

Thank you for this, that is a great catch. I feel like something should've caught this, but falsy is as falsy does...

[feywind]

However, the original WaitForProcessing feature was released as default behavior and this behavior change came with a minor version increment to 5.2.0. I would propose to release this as 6.0.0 or 5.3.0 instead of 5.3.1 since the default behavior change is a runtime relevant change for services consuming this client library.

I'm okay with it being a patch release since it's a bug in the intended behaviour. The fix would actually bring it closer to where it was before the shutdown options change.

I will double-check though.

[feywind]

I was hoping to avoid having to merge, but the test looks stuck.

[feywind]

There was a legit bug in the system test that this exposed. Two, really, if you count the Duration change from before. Yay! Unfortunately I will have to see who's around for approvals.

[nico-kn]

There was a legit bug in the system test that this exposed. Two, really, if you count the Duration change from before. Yay! Unfortunately I will have to see who's around for approvals.

Yay! Glad this helped! I would agree with your point about the patch release although it might make sense to still point it out in the changelog that only with the patch release the default behavior actually changed