The FIXED_xxx looks like something undone but it also points out what kinds of fixes you have done. It confirms what parts of the original code used to fail and how they behave correctly now. It's like the test auto-verifies the regression being fixed.
We can remove it in JDK 14. Are you OK with delaying "8228456: Enhance tests after JDK-8217375" to JDK 14? Thanks, Max > On Jul 29, 2019, at 12:12 AM, Philipp Kunz <philipp.k...@paratix.ch> wrote: > > Hi Max and John, > > Just saw it now and it sounds very reassuring that you say you reviewed all > the tests. Thank you so much for that non-trivial piece of work of quite some > amount. > > Now I see the FIXED_xxx stuff appearing in the head branch which it was not > intended to originally. For example: > http://hg.openjdk.java.net/jdk/jdk/file/5e637f790bb8/test/jdk/sun/security/util/ManifestDigester/FindSection.java#l54 > > In my opinion this should have been used to "calibrate the tests", meaning to > switch the flag, check where it is used manually and run the new tests > against the old src, in order to confirm that ManifestDigester's and other > classes's behavior has not changed in an undesired way, before merging it. > If that "calibration" still works now successfully, the flags and the code > becoming unreachable without them can and also should now be removed. > > This is probably something occurring hardly ever but ManifestDigester had > almost no test coverage before 8217375 and I would not feel comfortable > unless a reviewer confirms that all the new tests actually and correctly > conserve behavior that should not change as intended. See also > https://mail.openjdk.java.net/pipermail/security-dev/2019-July/020453.html. > > It's not exactly dramatic that the FIXED_xxx stuff has now found its way to > the head branch but it will never ever be useful again. It's also not exactly > urgent but could it be removed? This is most certainly exceptional but if I > may suggest it with all modesty and due respect, I'd see and welcome it as a > confirmation of a successful review of the "calibration" if someone else > would remove it. How does that sound or what would you or any policy or > existing procedure suggest? > > Regards, > Philipp > > > > On Sun, 2019-07-21 at 08:08 +0800, Weijun Wang wrote: >> Hi Philipp, >> >> I just took a brief look at the new patch. Looks like the src change has no >> behavior change (even in JarSigner.java with so many line changes). This is >> good. >> >> John and I have reviewed all the tests and we sent out some feedbacks in >> previous mails, and it's definitely a part of the whole review process. The >> single reason I integrated your changeset earlier this week is because RDP 2 >> starts at July 18 and after the date only P1-P2 src change would be approved >> but we can still modify the tests. Before pushing the change, I ran all >> tests, modified 2 regex patterns, and added one to problem list to make sure >> it makes no (or, as little as possible) noise (esp for other people running >> the tests). I haven't made any big change to the tests (including removing >> those FIXED_xxxxxxx flags) myself because I was still waiting for your >> reply. Thanks for coming back, so that we can start enhancing the tests now. >> Some test (Ex: PreserveRawManifestEntryAndDigest.java) still spends a lot of >> time and could intermittently time out. >> >> I'll file a new bug and post the interdiff between the integrated change and >> your new patch as the webrev.00 of it. >> >> Thanks, >> Max >> >>
