Concern on Testing with High Volume of File Modifications

[kiatng]

@fballiano raise these concerns:

Ref issue #4348:

Also, on the subject, do you think there is enough testing lately? hundreds and hundreds of files are modified almost daily, the errors came up after the PR in subject are very important so my doubts are relevant.

Ref issue #4291:

I think these massive PRs should be accepted only if they don't add new lines to the baseline. we know nobody is testing these modifications and there are hundreds of changed files every day. not having new lines is at least a basic indicator that maybe the PR itself is not introducing problems.

Following is my take:

First, the bug in the example was introduced in issue #4266. Are there any other known bugs from the hundreds of modifications in recent PRs? So far, this is the only one that I know of. Let's look at what happened:

Timeline:
10 OCT - issue #4266 created, 55 files
9 NOV - 2 green checks
11 NOV - merged to main
11 NOV - merged to Maho
12 NOV - bug reported, issue #4348
(today) 16 NOV - 1 green check #4348

Are the above events as it happened acceptable? I think it's within reason given our limited resources. But can we do better?

Second, the concern on large number of files in a PR and testing them. For PR that remove an entire module, PR #4274 (30 files), and where there are code changes, PR #4152 (120 files), we are careful. For PRs that are mainly doc block changes to comply with standards, PR #4291 (73 files), #4328 (135 files), #4283 (31 files) ; testing are not required but are reviewed. For the last category: standard complying modifications necessary? Yes, from a coder perspective, it's very beneficial for coders . Not so much from users perspective.

Credit to @sreichel, whom had put in a lot of efforts on these PRs and added a bunch of unit tests.

I hope I have addressed these concerns.

[sreichel]

So far, this is the only one that I know of

We had more lately.

• Bugfix: cant create order from admin backend #4348 ... fix for Make Mage_GiftMessage optional in templates #4266
• Bugfix: cant view order in customer account #4345 ... fix for Make Mage_GiftMessage optional in templates #4266
• Bugfix: hide exception, ref #4303 #4344 ... not a bug, but throws exceptions in dev-mode Code quality: added PHPMD, updated workflow #4303
• Hotfix: core_store.store_id can be null #4330 ... fix for Added more PhpUnit tests #4236
• Better sitemap configuration #4308 ... contains fix for Admin config: added input validation & hide disabled options #4232

[addison74]

At first glance the concerns expressed by a former OpenMage maintainer could be considered justified. As far as I'm concerned, I don't approve without doing a local PR test. Indeed there were PRs with many modified files, most of them related to errors reported by PHPStan. I approved them not before using PHPStan locally in a DDEV environment. That PR for issues in the Gift module was a complex one and the changes needed to be verified better. Fortunately, the problem was quickly discovered and fixed immediately. Let's not lose sight of the fact that we haven't released any new version in the meantime to affect those who upgrade. I would recall what happened when the WSDL value changed and how many discussions it generated.