Hi John,
Let me skip most of cosmetic/style comments. I think you got my points,
I'll let you decide if you want to address them or not. Or, you may want
to ask for other's opinions.
Let's focus on the question about timeout below.
On 11/24/2017 08:57 AM, sha.ji...@oracle.com wrote:
Could you please upload the report to
http://cr.openjdk.java.net/~jjiang/8186057 ?
http://cr.openjdk.java.net/~jjiang/8186057/webrev.05/report/report.html
Thank you. It might be better to add some space between columns.
I add a style for the table and make it more friendly.
http://cr.openjdk.java.net/~jjiang/8186057/webrev.06/report/report.html
The report looks much better now, thank you.
One question about Compatibility.caseStatus(). What's the case when
you expect a timeout and no client output? Should it always result
to a test case failure?
I'm not sure understanding your question. Did you mean server or
client timeout?
I think client should output something.
My questing is - should timeout be always considered as a failure?
Why don't you just check if server or client result is FAIL?
if (clientStatus == FAIL || serverStatus == FAIL) {
return FAIL;
}
return PASS;
Some cases are negative cases, their status should be EXPECTED_FAIL.
That's fine, I got that.
A server or client timeout would lead to the case to fail, if the
failure is not expected.
If a server starts normally, but client doesn't start due to a
negative case, the server should get timeout. But this failure is
expected.
If a server starts normally, and client get a unexpected timeout, this
failure would make the case to fail.
This logic looks unnecessary complex to me. The more complex it is, the
more likely we get a bug there. I think TIMEOUT status is not necessary
here. If a failure is expected on one side, the other side should fail
as well (no matter if it's a timeout or an exception).
If I were you, I would simplify this method like this
if (client == EXPECTED_FAIL && server == EXPECTED_FAIL) {
return EXPECTED_FAIL;
}
if (client == PASS && server == PASS) {
return PASS;
}
return FAIL;
If I am not missing something, the pseudo-code above looks better and
simpler.
Otherwise, looks fine to me.
A couple of minor comments which you may want to address:
4. "sequence" field of JdkRelease looks redundant. A enum already
defines the order. The same with Protocol.
It looks the ordinal of enum constants is not recommended [1].
[1]
https://docs.oracle.com/javase/9/docs/api/java/lang/Enum.html#ordinal--
I don't see that it says that using "ordinal" is not recommended. I
think "sequence" just duplicates of "ordinal".
The following words are copied from the description of method
Enum.ordinal():
"Most programmers will have no use for this method. It is designed for
use by sophisticated enum-based data structures, such as EnumSet and
EnumMap."
I don't think it says that "ordinal" is not recommended to use :)
In addition, Effective Java (2nd) Item #33 also advise us not using
ordinal index.
I respect and appreciate Mr. Bloch's book, but my point is that
"ordinal" and "sequence" have exact the same meaning, so "sequence" just
duplicates it, and as a result, it looks redundant :)
http://hg.openjdk.java.net/jdk10/master/file/be620a591379/src/java.base/share/classes/java/lang/Enum.java#l91
...
private final int ordinal;
...
public final int ordinal() {
return ordinal;
}
...
Artem
Best regards,
John Jiang
