Bug 493136

Comment 1 Tamas Miklossy 2016-05-06 08:58:27 EDT

Created attachment 261510 [details]
Invoke the Dot Attribute validation consistenly

I uploaded a patch with the following improvements:

[493136]: Invoke the Dot Attribute validation consistently for setters

- Extend the setForceLabels method with validation method invocation.
- Remove superfluous period at the end of the error message when parsing
invalid "dir" attribute value.
- Change the error message for setting invalid type attribute to be
consistent with the other Dot attribute error messages.
- Use the attributesProperty() consistently (instead of the
getAttributes()) to get/set a given Dot attribute value.
- Add missing "throws IllegalArgumentException" javadoc for the
DotAttributes setter methods.
- Implement additional DotAttribute Test cases for the "distortion",
"forcelabels" dot attributes (getter/setter/getParsed/setParsed method
with valid and invalid values).

Hereby I confirm that my contribution complies with https://www.eclipse.org/legal/CoO.php.

Comment 2 Tamas Miklossy 2016-05-06 09:35:07 EDT

Created attachment 261511 [details]
Invoke the Dot Attribute validation consistenly

I uploaded a patch with the following improvements:

[493136]: Invoke the Dot Attribute validation consistently for setters

- Extend the setForceLabels method with validation method invocation
- Remove superfluous period at the end of the error message when parsing
invalid "dir" attribute value.
- Use the attributesProperty() consistently (instead of the
getAttributes()) to get/set a given Dot attribute value.
- Add missing "throws IllegalArgumentException" javadoc for the
DotAttributes setter methods.
- Implement additional DotAttribute Test cases for the "distortion",
"forcelabels" dot attributes (getter/setter/getParsed/setParsed method
with valid and invalid values).

Hereby I confirm that my contribution complies with https://www.eclipse.org/legal/CoO.php.

Comment 3 Alexander Nyßen 2016-05-06 09:43:26 EDT

Thanks Tamas, looks good. I released your patch to origin/master. I will leave this open, as the validation tests are not yet complete. I suppose you will contribute some additional tests?

Comment 4 Tamas Miklossy 2016-05-07 07:59:44 EDT

Created attachment 261535 [details]
Implement additional DotAttributes test cases

I uploaded a patch with the missing Dot Attributes test cases implementation:

[493136] Implement additional DotAttributes test cases

- Rename DotAttributes.LP__E to DotAttributes.LP__GE to indicate that
the lp (label position) attribute is valid for both graphs and edges.
- Implement missing getLp/getLpParsed/setLp/setLpParsed methods for
graphs.
- Add the missing validation method invocation within the setFixedSize,
setHeadLp, setLp, setTailLp, setXlp methods (also extend the
DotJavaValidator logic).
- Improve javadoc of the getLayoutParsed(Graph graph) method.
- Implement DotAttributes test cases for the "label", "layout", "lp",
"shape", "skew", "sides", "style" dot attributes
(getter/setter/getParsed/setParsed methods with valid and invalid
values).

Hereby I confirm that my contribution complies with https://www.eclipse.org/legal/CoO.php.

Comment 5 Alexander Nyßen 2016-05-09 09:21:10 EDT

Thanks Tamas! I released your patch to origin/master. I think we are almost done here. What should still be added, IMHO are interpreter/import tests for the individual attribute values, so we ensure they are all handled properly by the interpreter.

Comment 6 Tamas Miklossy 2016-05-09 10:42:47 EDT

Created attachment 261562 [details]
Create DotSampleGraphs.xtend with sample graph definitions

Yes, Alexander, you are right, some DotImporter/DotInterpreter test cases are still missing. I think first of all we should clean up these test cases before implementing the missing ones.

I uploaded a patch with the following improvements:

[493136]: Create DotSampleGraphs.xtend with sample graph definitions.

- Extract the definitions of the sample dot graphs into a separate
DotSampleGraphs.xtend file (utilize the xtend multi-line string functionality).
- Reuse these sample graph definitions within the DotParser and
DotInterpreter test cases.
- Modify the org.eclipse.gef4.dot.tests plugin to be able to handle the
xtend generated files (.classpath, .gitignore, .project, build.properties, pom.xml).
- Implement additional DotParser test cases.

Hereby I confirm that my contribution complies with https://www.eclipse.org/legal/CoO.php.

Comment 7 Alexander Nyßen 2016-05-10 01:13:58 EDT

Thanks Tamas. Nice idea to use Xtend here. I released your patch to origin/master.

Comment 8 Tamas Miklossy 2016-05-10 11:09:36 EDT

Created attachment 261603 [details]
DotImportTests clean-ups and extensions

I cleaned up the DotImport test cases and also defined some new ones:

[493136] DotImportTests clean-ups and extensions

- Divide the testFileImport() test case into smaller test cases (define one test case for each sample .dot file).
- Extend the existing dot import test cases with org.eclipse.gef4.graph.Graph toString() equality assertions (for some sample files, the resulting graph was only checked whether it is null or not).
- Implement additional dot import test cases based on the following sample files: arrowshapes_deprecated.dot,arrowshapes_direction_both.dot, arrowshapes_multiple.dot, arrowshapes_single.dot.

Hereby I confirm that my contribution complies with https://www.eclipse.org/legal/CoO.php.

Comment 9 Alexander Nyßen 2016-05-10 11:38:45 EDT

(In reply to Tamas Miklossy from comment #8)
> Created attachment 261603 [details]
> DotImportTests clean-ups and extensions
> 
> I cleaned up the DotImport test cases and also defined some new ones:
> 
> [493136] DotImportTests clean-ups and extensions
> 
> - Divide the testFileImport() test case into smaller test cases (define one
> test case for each sample .dot file).
> - Extend the existing dot import test cases with
> org.eclipse.gef4.graph.Graph toString() equality assertions (for some sample
> files, the resulting graph was only checked whether it is null or not).
> - Implement additional dot import test cases based on the following sample
> files: arrowshapes_deprecated.dot,arrowshapes_direction_both.dot,
> arrowshapes_multiple.dot, arrowshapes_single.dot.
> 
> Hereby I confirm that my contribution complies with
> https://www.eclipse.org/legal/CoO.php.

Thanks Tamas. However, I am unsure if this is the right strategy to improve the test basis. The test resource files are not very systematic, most of them were contributed as examples. 

I would propose to leave only few file-based tests, which should explicitly target the different file-based aspects (e.g. proper escaping of \\) and to transfer the other test resources into more consistent tests that are based on DotSampleGraphs (which we should probably rename to DotTestGraphs or similar), so we can design the (interpreter related) tests in a systematic manner (e.g. a test for each attribute).

Comment 10 Tamas Miklossy 2016-05-12 13:58:51 EDT

Created attachment 261698 [details]
Add test dot graphs (DotSampleGraphs.xtend) with global/local/override dot attributes

I agree with you, so I created a patch with additional DotSampleGraphs.

[493136] Add test dot graphs with global/local/override dot attributes

- define sample graphs with global/local/override dot attributes for all
supported edge/node attributes except the following ones (since the
global definition of these attributes does not make any sense):

edge_headlp()
edge_id()
edge_lp()
edge_pos()
edge_taillp()
edge_xlp()
node_id()
node_pos()
node_xlp()

Hereby I confirm that my contribution complies with https://www.eclipse.org/legal/CoO.php.

Comment 11 Alexander Nyßen 2016-05-12 15:33:29 EDT

(In reply to Tamas Miklossy from comment #10)
> Created attachment 261698 [details]
> Add test dot graphs (DotSampleGraphs.xtend) with global/local/override dot
> attributes
> 
> I agree with you, so I created a patch with additional DotSampleGraphs.
> 
> [493136] Add test dot graphs with global/local/override dot attributes
> 
> - define sample graphs with global/local/override dot attributes for all
> supported edge/node attributes except the following ones (since the
> global definition of these attributes does not make any sense):
> 
> edge_headlp()
> edge_id()
> edge_lp()
> edge_pos()
> edge_taillp()
> edge_xlp()
> node_id()
> node_pos()
> node_xlp()
> 
> Hereby I confirm that my contribution complies with
> https://www.eclipse.org/legal/CoO.php.

Which of the sample models can be replaced thereby? Shouldn't we adjust the tests to use these new samples as well?

Comment 12 Tamas Miklossy 2016-05-12 15:38:13 EDT

I think these sample graphs would be a good basis for further DotImportTest/DotInterpreterTest definitions.

Comment 13 Alexander Nyßen 2016-05-13 05:21:35 EDT

(In reply to Tamas Miklossy from comment #12)
> I think these sample graphs would be a good basis for further
> DotImportTest/DotInterpreterTest definitions.

Ok. I have released your patch to origin/master.

Comment 14 Tamas Miklossy 2016-05-13 05:47:37 EDT

Created attachment 261715 [details]
Add missing DotInterpreter logic for node attributes handling

I uploaded a patch with the following improvements:

[493136] Add missing DotInterpreter logic for node attributes handling

- The following dot attributes were not handled properly by the dot
interpreter: "distortion", "shape", "sides", "skew", "style".
- Implement additional DotImport Test cases based on the test graphs
defined within the DotSampleGraphs.xtend file.

Hereby I confirm that my contribution complies with https://www.eclipse.org/legal/CoO.php.

Comment 15 Alexander Nyßen 2016-05-13 08:43:20 EDT

Thanks Tamas! I released your patch to origin/master. I have performed some additional cleanups and think we are done here then. 

Resolving as fixed in 4.0.0 RC1.