Alan, I have update the fix with these changes: 1. For error text verification, whenever appropriate, I am using regex, such as this "in both module m. and m.”. This, obviously is a weaker check, as it allows invalid outputs such as "in both module m2 and m2” and others, but I believe it is acceptable for this case. If there will be more cases similar to this, I would create a library method later, which would do a stricter check.
2. Removed the testOverlapUpgradePath for now. 3. Fixed the summary. 5. Fixed spaces and tabs - awfully sorry about that. I have also added one more case into OverlappingExportedPackagesTest. Shura On Oct 1, 2015, at 6:23 PM, Alan Bateman <alan.bate...@oracle.com> wrote: > On 23/09/2015 15:45, Alexandre (Shura) Iline wrote: >> Hi. >> >> This change adds a few more cases to existing cases for overlapping packages: >> http://cr.openjdk.java.net/~shurailine/webrev.overlapping.packages.2/ >> >> > Sorry for the late reply, too many things going on. I have a few comments: > > 1. I agree it's good to check the output in addition to checking for a non-0 > exit. However it turns out to be fragile: it might be "in both module m1 and > m2" on one system and "in both module m2 and m1" on another. This is because > the system module path varies by platform and build configuration. > > 2. testOverlapUpgradePath assumes that the upgrade module path can be used as > an alternative to the application module path. There are several discussion > points around the upgrade module path but I don't expect it can be used to do > anything other than upgrade modules that are on the system module path (or > linked into the runtime image). So maybe this one should be dropped for now. > > 3. The @summary in OverlappingPackagesTest should be used as the updated test > includes positive scenarios now. > > 4. I'm sure that that OverlappingExportedPackagesTest is the best name for > this test because it is testing several negative scenarios that are specified > by Configuration.resolve. I think we've already got each of these cases > covered in ConfigurationTest but good to have them tested at startup for the > boot layer too. > > 5. There are trailing spaces and tabs in the patch. > > -Alan.
