Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 232669

Summary:

Regression - bug in OCLAbstractAnalyzer

Product:

[Modeling] OCL

Reporter:

Adolfo Sanchez-Barbudo Herrera <adolfosbh>

Component:

Core

Assignee:

Christian Damus <give.a.damus>

Status:

CLOSED FIXED

QA Contact:

Severity:

critical

Priority:

Version:

1.2.0

Target Milestone:

Hardware:

OS:

Linux

Whiteboard:

Attachments:

Description	Flags
contributed Test Cases	none
Fix with different test approach	none

Description Adolfo Sanchez-Barbudo Herrera

2008-05-17 13:55:06 EDT

Created attachment 100781 [details]
contributed Test Cases

The analyzer throws a ClassCastException, when calling the feature of a collection or multievaluted property which doesn't exist for the element type of the element:

aCollection.nonExistingProperty

The problem is when trying to create an implicitCollect (which is done because the source of the featureCallExp has a collectionType). If the featureCall doesn't exist, a type casting from an InvalidadLiteralExp to a FeatureCallEx is made, and an exception is thrown.

Maybe the collect iteratorExp might not be created, if the featureCallExp is an invalidLiteralExp.

I think that the regression came from the following bug resolution:

https://bugs.eclipse.org/bugs/show_bug.cgi?id=226083 

I have attached a test case for uml and ecore, which can be contributed to take this case into account in further developments.

I don't have too much experience with OCL test case infrastructure. So feel free to refactor it as much as you consider.

Cheers.

Comment 1 Christian Damus

2008-05-17 16:39:50 EDT

Created attachment 100784 [details]
Fix with different test approach

A fix, with a different approach to testing the problem.  I use the fact that a "normal" parse failure is recorded as a Diagnostic in the OCLHelper to verify that the parse did not fail because of an exception.

Comment 2 Christian Damus

2008-05-17 16:45:39 EDT

Fix committed to CVS HEAD (1.2 release).

Thanks, Adolfo, for making the effort to submit JUnit tests.  As it happens, in this case I was not able to use them because I found a mechanism that I prefer over checking for message text (which is subject to change).

Comment 3 Christian Damus

2008-05-17 16:46:37 EDT

Comment on attachment 100781 [details]
contributed Test Cases

Marking the patch as obsolete because I did not end up using it.

Comment 4 Adolfo Sanchez-Barbudo Herrera

2008-05-17 17:23:03 EDT

Hi Christian,

Don't worry, I'm just learning :). I certainly didn't like my test case. Using the message of the exception, is not very recommended (for example if the text of the problem changes, the test fails), but It was the first one which I could get to pass M6, and fail M7. 

I was very sure that you were changing it ;).

One question more. AFAIU understand the test case and the new assetNoException method, the test case just fails if ClassCastException fails.

The test itself is useful to show the problem before the patch. But it's not useful to detect further problems with the same case. I would have expected some a ssertion (assertException) of the expected problem, and if the exception is different from the expected one, then the test case fails.

Maybe going through all the nested exception, and checking that every exception is not different from expected one, would be better....

I could provide an assertException method with the related behaviour, and apply it to all that cases which expects exceptions from the parser. The test cases should behave better (detecting problems) if any side-effect change is introduced.

Cheers,
Adolfo.

Comment 5 Christian Damus

2008-05-18 13:44:53 EDT

Hi, Adolfo,

You raise a good point.  In this case, however, the check for ClassCastException in the test is actually redundant.  Possibly it should be RuntimeException, but that would only actually be useful when (if) the parser starts to produce Diagnostics when parsing fails with exceptions.  Where this test actually fails with the M7 code is in the assertion that there is a Diagnostic in the first place.  For now, that will catch any future run-time exceptions.

Comment 6 Ed Willink

2011-05-27 02:48:10 EDT

Closing after over 18 months in resolved state.