On 10/30/23 03:40, Yao, Jiewen wrote: > Thanks Mike. I am reading the WIKI page. > > >> and/or provides testing or regression testing for the Package (or some >> modules thereof), in certain platforms and environments. > > [Jiewen] Are we expecting Reviewer to provide testing or regression testing > for the package? > Is that what the reviewer *commits* to do? > For example, Maintainer may ask the reviewer to do some testing, right?
It depends on the reviewer's individual commitment. First of all, the burden of testing / regression-testing, to a reasonable extent [1], is on the submitter. [1] In some cases, the submitter cannot test the code they modify in all possible environments / circumstances. In such cases, the submitter should test the change code wherever they can, as widely as they can, and be upfront (in the commit message) about lacking coverage in other environments they might be aware of. It is then fine for the maintainer (or even reviewer) to ask others for further / wider testing, but trying to saddle someone with that testing as an *obligation* will never fly. That would only alienate people from contributing. This was the primary reason for splitting Xen code as sharply as possible from non-Xen code in both ArmVirtPkg and OvmfPkg. Xen and QEMU/KVM are so different environments, with so distinct audiences, that keeping code common was *worse* than duplicating and customizing code. We could never *sufficiently* regression-test our changes for each other. It was best to separate those areas of interest. Demanding that I regression-test on Xen, or that Anthony or Julien regression-test on QEMU/KVM, would have lead nowhere. Second, the level of commitment varies. A reviewer may have a fleeting interest in a module (just want to be in the loop), or else they may be completely invested in it, and they might actually prefer being a maintainer. For example, I had seen many bad regressions in OVMF due to UefiCpuPkg patches, thus, even thouogh I absolutely didn't welcome the additional responsibility, I asked to be added as a Reviewer for UefiCpuPkg. With that, I wanted to formalize my request to be CC'd on all UefiCpuPkg patches, but I also committed to regression testing, and maybe even reviewing, those patches. It worked out quite well, but of course I was still selective in what I would review and test. If I could immediately determine that the patch modified code in UefiCpuPkg that never ran on (or wasn't even built into) OVMF, I would clearly state on the list that I'd not review or test the patch, i.e., that nobody should wait for me. > > >> Reviewer is responsible for timely responses on emails addressed to them >> (preferably less than calendar week). > > [Jiewen] > Is that what the reviewer *commits* to do? What I think we can expect a reviewer to *commit* to is to say *something* reasonably quickly. The whole idea is to support others in making a *decision*, in making progress. So the "something" the reviewer says may well be: - "this does not apply to the area I have expertise or interest in, so please proceed with this patch without my feedback (testing or review or opinion etc)" - "I don't have time for this right now, so please go ahead; if it breaks, we'll figure it out later" (the maintainer need not accept this, and might want to block the patch independently for a bit longer, until someone else provides the desired review or testing, but the reviewer is still totally OK to say this) - "please give me a few more days to review this set". > For example, Maintainer may ask the reviewer to provide feedback, right? IMO, the maintainer is welcome to request feedback, of course; that's presumably why the reviewer wanted to be listed in Maintainers.txt in the first place. Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#110305): https://edk2.groups.io/g/devel/message/110305 Mute This Topic: https://groups.io/mt/102245264/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
