Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Type checks not functional any more (since first CobiGen release) #40

Closed
5 tasks done
maybeec opened this issue Sep 29, 2014 · 20 comments
Closed
5 tasks done

Type checks not functional any more (since first CobiGen release) #40

maybeec opened this issue Sep 29, 2014 · 20 comments

Comments

@maybeec
Copy link
Member

maybeec commented Sep 29, 2014

Due to the migration of the APPS-Generator's single application design to a more modularized architecture, the type checking template methods of the java plug-in have been destroyed.

The problem is, that the whole model will be converted to a DOM model to enable xPath expressions on the model within the template. As of the modularization, the type checking template methods are now part of the model itself and not part of the global variables of FreeMarker. Therefore, the needed ability to integrate a BeanModel for the util implementation in a generic way has been eliminated.

Thus we have to find a new approach to handle this. My current proposal would be to add another method for InputReaders to be called beside the createModel() method. This method then might provide multiple function extensions for the template. I would also suggest to use the right extension point of FreeMarker for functions.

@maybeec
Copy link
Member Author

maybeec commented Oct 22, 2014

Please also document potential changes in the dependencies done during development in the licence tracker: https://github.com/oasp/tools-cobigen/wiki/mgmt_dependency-and-license-tracking
Just add a new table for the new version of the java plugin at the bottom. Document the final target version here (without snapshot).

fkreis pushed a commit that referenced this issue Oct 23, 2014
…e message)

fixed casting problem: template methods are called with SimpleScalars instead of Strings
fkreis pushed a commit that referenced this issue Oct 23, 2014
@fkreis
Copy link
Contributor

fkreis commented Oct 23, 2014

I think I didn't change the dependencies. I only used the TemplateMethodModelEx interface which belongs to Freemarker and so already is documented.

@maybeec
Copy link
Member Author

maybeec commented Oct 23, 2014

But you have changed the dependency of cobigen-core from v1.0.0 to v1.1.0, haven't you? ;)

@fkreis
Copy link
Contributor

fkreis commented Oct 23, 2014

ok added it.
What do you mean with: "Document the final target version here (without snapshot)."

@maybeec
Copy link
Member Author

maybeec commented Oct 23, 2014

I only indicated that you do not state v1.1.0-SNAPSHOT as the new dependency :) But you have done it intuitively right :)

@fkreis
Copy link
Contributor

fkreis commented Oct 23, 2014

ahhh :)
so if you are satisfied with the results we can close this issue.
(Maybe we shoud remove the old class FreemarkerUtils and update the tests using that class)

@fkreis
Copy link
Contributor

fkreis commented Oct 23, 2014

oh... The new testcases test only the freemarkerutil methods but not JavaInputReader.getTemplateMethods(Object). I will add that one...

@maybeec
Copy link
Member Author

maybeec commented Oct 23, 2014

I think the old FreeMarkerUtil can be removed completely as it is not used any more. Same for the tests of it. You will have your own tests testing the new templates methods.
I will have a look at the results tomorrow morning. If you at the university, we can talk about open issues or I will close the issues otherwise.

fkreis pushed a commit that referenced this issue Oct 24, 2014
…dTest into small single testcases.

+ added some javadoc in IsSubtypeOfMethod
fkreis pushed a commit that referenced this issue Oct 24, 2014
fkreis pushed a commit that referenced this issue Oct 24, 2014
@fkreis fkreis closed this as completed Oct 24, 2014
@maybeec
Copy link
Member Author

maybeec commented Oct 24, 2014

During your code cleanup you killed test JavaInputReaderTest.provideParsingAndReflectionModelFeatures :)
Unfortunately we currently do not have any continuous integration with running nightly builds. Nevertheless, you might want to run all tests by running the mvn clean install command. This will run all tests and fail if there are some tests failing.

Can you correct the failing test?

@maybeec maybeec reopened this Oct 24, 2014
@fkreis
Copy link
Contributor

fkreis commented Oct 24, 2014

yes sure ; )

maybeec added a commit that referenced this issue Oct 25, 2014
…gories + Integration tests written for template methods
@maybeec
Copy link
Member Author

maybeec commented Oct 25, 2014

Unfortunately I found another issue during integration test. Maybe we should also introduce integration tests as you proposed.

I refactored the test organisation a little bit to distinct unit test from integration tests. I also added a test suite in the integrationtest package, which includes integration tests for the two methods implemented, which are currently not valid. Please can you have a look at them and green the test runner again? :) I added the integrationtest by myself to give you an example how to do such integration tests for plug-ins in future.

You may want to have a look at the isValidInput(Object) method to get to know all possible input types handled by the JavaInputReader.

@maybeec maybeec reopened this Oct 25, 2014
fkreis pushed a commit that referenced this issue Oct 27, 2014
@fkreis
Copy link
Contributor

fkreis commented Oct 27, 2014

Now the two integration tests are valid, so I think the template methods should work for inputs of type class now. It also should work for valid Object[] with length = 2.

PackageFolder has a method getClassLoader but potentially it returns null because its initialized in the constructor. There is one Constructor PackageFolder(URI location, String packageName, ClassLoader classLoader) but the parameter classLoader is never used!

There is no solution for input of JavaClass until now.

@maybeec
Copy link
Member Author

maybeec commented Oct 27, 2014

So if the classLoader field of PackageFolder is null, just use the current classLoader your application is running with. This field might not be used anymore because you removed the FreeMarkerUtil already :)

@fkreis
Copy link
Contributor

fkreis commented Oct 28, 2014

how should we deal now with instances of JavaClass?

@maybeec
Copy link
Member Author

maybeec commented Oct 28, 2014

I had a short look at JavaParserUtil#getFirstJavaClass, where the classLoader is appended while building a sourceLibrary and parsing the Java source files. But I think after lossing the reference to the ClassLibraryBuilder, we also loose the classLoader reference.
You might want to have also a look at it, but if there is no way to get the original ClassLoader reference anymore for parsed content, we should go for the default classLoader.

@maybeec
Copy link
Member Author

maybeec commented Oct 29, 2014

  • see comment in 2017649
  • fix constructor of PackageFolder

fkreis pushed a commit that referenced this issue Oct 29, 2014
@fkreis fkreis closed this as completed Oct 29, 2014
maybeec added a commit that referenced this issue Oct 29, 2014
@maybeec
Copy link
Member Author

maybeec commented Oct 29, 2014

Due to some more integration testing, I found further issues.
See the latest committed test. Please fix, if possible. Otherwise, just note me, that I have to do it before releasing.

@maybeec maybeec reopened this Oct 29, 2014
@fkreis
Copy link
Contributor

fkreis commented Oct 30, 2014

the problem is, that the implemented methods expect an arg which is a list of Strings, but e.g. variables.abstractCollection in the template isAbstract.ftl provides an ElementModel and not the String "java.util.AbstractCollection" as intened by the context.xml, what I do not understand at the moment.

@maybeec
Copy link
Member Author

maybeec commented Oct 30, 2014

I think this is FreeMarker internal stuff we have to fight with.
So in general, you should check the type of the input when you are casting. There is no type check performed by FreeMarker, so we have to do that. Furthermore, if input parameter does not match the expected types, you might want to throw an exception with an appropriate message.
Currently, the user gets a ClassCastException, which is most likely interpreted as a bug.

As far as I got the idea of the FreeMarker types, a SimpleScalar is something like string, int, long, ...
An ElementModel then is some expression including FreeMarker stuff. As far as I got it during a short debug, we should get the evaluated value from the ElementModel expression by some getter. So the only thing to do is to handle such new input types. I provided different expressions as parameters for the template methods in the test. I think that are the most commonly used ones. If you might imagine further ones, try them in the test.

@fkreis
Copy link
Contributor

fkreis commented Nov 3, 2014

the failing test testCorrectClassLoaderForMethods resulted from an fault caused by me, sry 4 that : )
the other 2 (testIsAbstractMethod() and testIsSubtypeOfMethod()) resulted from type problems as described before. Therefore added type checks and casts to NodeModel/String if possible. Otherwise an exception is thrown now.

@fkreis fkreis closed this as completed Nov 3, 2014
@fkreis fkreis removed their assignment Nov 16, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants