Hi Chris,
Thank you for the review.
So we have 2 ways - create new RFE for refactoring and then fix this bug
in updated code.
or just fix this 2 tests without refactoring (the changes in the tests
will be identical).
Do you think it makes sense to go #1 or just do #2?
Regarding using Platform.isWindows - it's good for the case, I'll fix it
in the next iteration.
--alex
On 02/06/2020 15:01, Chris Plummer wrote:
Hi Alex,
When refactoring is big and the bug fix is small, I prefer to see the
refactoring done first. It just keeps things cleaner and makes it easier
for the reviewer to see the important changes. It also helps anyone
looking at this bug or these tests in the future to better recognize
what the actual bug fix was, and what was just refactoring. Think if
there was another test with this issue, and someone was looking at the
diff of this fix to see how to apply it to the other test.
BTW, there is already a Platform.isWindows() API. It should probably be
used rather than the check the test is using. It is a slightly different
test however, testing for a prefix of "win" rather than "windows"
anywhere in the string.
thanks,
Chris
On 2/6/20 1:14 PM, Alex Menkov wrote:
Hi all,
Please review the fix for
https://bugs.openjdk.java.net/browse/JDK-8234935
webrev:
http://cr.openjdk.java.net/~amenkov/jdk15/JdwpTestsTeredo/webrev/
The failures are caused by Teredo clients
(https://en.wikipedia.org/wiki/Teredo_tunneling).
The fix filters out corresponding addresses.
JdwpListenTest and JdwpAttachTest use the same way to get addresses
for testing. As this is not the 1st time the algorithm is updated I
decided to deduplicate the code and move shared code to new base class.
So actual change is the addition of
71 // Teredo clients cause intermittent errors on listen ("bind failed")
72 // and attach ("no route to host").
73 // Teredo is supposed to be a temporary measure, but some test
machines have it.
74 if (isTeredo(addr6)) {
75 continue;
76 }
and isTeredo method implementation.
--alex
