Use status severity and codes in JDT model test assertions

[fedejeanne]

An alternative to #5005 .

Do the checks/assertions based on the status and the error code instead of the text.

[fedejeanne]

@trancexpress I think these kind of assertions would make the tests less brittle.

In general, I try to write tests in a way that they do not check for (error) messages unless of course the sole purpose of the test is to validate the message. These tests are not interested in the message itself but in the kind of error produced by the operations.

WDYT?

[trancexpress]

From my POV its OK for these tests to check with the status error code instead of the message.

[fedejeanne]

In that case, if you decide to merge this one then #5005 may be dropped👍

[trancexpress]

Check warning on line 1823 in org.eclipse.jdt.core.tests.model/src/org/eclipse/jdt/core/tests/model/JavaProjectTests.java

@jenkins-eclipse-jdt jenkins-eclipse-jdt / Compiler

Unnecessary Code

NORMAL:
The value of the local variable rootPath is not used

Should we just merge #5005 ?

[trancexpress]

#5005 got merged, do we continue here?

[fedejeanne]

@trancexpress I am at the OCX this week so I won't be able to look at it. I can continue this PR next week so please don't close it :-)

[fedejeanne]

I rebased on master and I also added a 2nd commit that removes unnecessary code (discovered by the checks in this PR)

[fedejeanne]

Still interested on this one, @trancexpress ? :-)

[trancexpress]

Still interested on this one, @trancexpress ? :-)

I don't think ClasspathInitializerTests cares about the actual message, but the actual message is not tested anywhere else. That does make me slightly uncomfortable with the change.

So one more item from me: Can you check if JavaModelStatus can be used to create the expected status, from the error code you are already providing? Then the message check can also be retained. That will shift the responsibility to JavaModelStatus to create the proper status.

[fedejeanne]

Then the message check can also be retained.

You mean checking the text of the error message in the tests too like #5005 did? The purpose of this PR was the exact opposite: to not check the messages since the most important thing is the kind of generated error. Or did I misunderstood your comment?

[fedejeanne]

ping @trancexpress

[trancexpress]

to not check the messages since the most important thing is the kind of generated error

For me as a user, the error kind doesn't help much. I can imagine seeing the right error kind in the test, but the wrong message.

If we can at least shift the responsibility somewhere, e.g. ask JavaModelStatus for the message (passing some parameters to it), IMO it would be safer on the long run. I.e. assert that the status has the right message, where the message is provided by JavaModelStatus.

This can still backfire if JavaModelStatus is faulty, but I guess if we want no hard-coded error messages in the tests, not much we can do. Maybe we can use the org.eclipse.jdt.internal.core.util.Messages, if we want to be surer.

[fedejeanne]

For me as a user, the error kind doesn't help much. I can imagine seeing the right error kind in the test, but the wrong message.

I agree with that but the test is not about what the user sees (the message), it is about Did this error happen (yes/no)?. Which is why I propose to let the test pass/fail based on whether or not the error happens and let the message be merely what org.eclipse.jdt.internal.core.util.Messages says it is.

If we can at least shift the responsibility somewhere, e.g. ask JavaModelStatus for the message (passing some parameters to it), IMO it would be safer on the long run. I.e. assert that the status has the right message, where the message is provided by JavaModelStatus.

IIUC you propose to do something like this (pseudo-code)

String expectedMessage = JavaModelStatus.getMessage(IJavaModelStatusConstants.INCOMPATIBLE_JDK_LEVEL);
assertStatus(
   "should have complained about jdk level mismatch",
   expectedMessage,
   status);

right?

In that case, the logic would be the same I have right now because one would get the expectedMessage from JavaModelStatus, which would give the message according to status.getCode(), which is exactly what this method asserts in its second line:

protected void assertStatus(String message, int expectedSeverity, int expectedCode, IStatus status) {
	assertStatus(message, expectedSeverity, status);
	assertEquals(message, expectedCode, status.getCode());
}

... or did I get you wrong?
Could you maybe put it in code so I can understand it better?