CAMEL-13979 - create camel-robotframework component by onderson · Pull Request #3172 · apache/camel

onderson · 2019-09-15T20:40:26Z

camel-robotframework component: rebase and polish
add karaf feature for robotframework component
docs and cleanup
Fix CS

oscerd

Can you please add a Spring Boot Integration test too? LGTM, just minor things.

oscerd · 2019-09-16T06:15:28Z

      <artifactId>camel-ribbon</artifactId>
      <version>${project.version}</version>
    </dependency>
+    <dependency>


Here you are missing the stater

oscerd · 2019-09-16T06:16:10Z

+### Options
+
+// component options: START
+


I think this must be populated anyway.

i ran full build but those were not populated. doing it again. let me see if they will get populated. will update soon.

Is not going to work I guess, I think there is a bug in the maven plugin, when it checks for the for the Schema JSON file that is generated from the schema endpoint parameter, it checks with case sensitivity, since your schema is scheme = "RobotFramework", it failed to load the json file since it was expecting the name to be without uppercase robotframework.json. To workaround it, can you please change the schema name to robotframework and run the full build again? I will fix that small bug later

Update: I was wrong here, in fact your schema name should be in small case

oscerd · 2019-09-16T06:17:30Z

+@UriParams
+public class RobotFrameworkCamelConfiguration implements Cloneable {
+
+    /**


This can be moved to getter/setter section, same for all the javadoc

oscerd · 2019-09-16T06:18:33Z

+
+public interface RobotFrameworkCamelConstants {
+
+    public final String ROBOT_CAMEL_EXCHANGE_NAME = "exchange";


It doesn't seem generic enough as name, same for the others ROBOT_VAR

moved them into utils and left what's used to set headers and whatnot..

oscerd · 2019-09-16T06:20:00Z

+# limitations under the License.
+#
+
+class=org.apache.camel.component.robotframework.RobotFrameworkComponent


In camel 3 this is not needed anymore, it will be autogenerated during the build

removed that bit.

omarsmak · 2019-09-16T11:20:01Z

+
+        if (configuration.getXunitFile() == null) {
+            String testCasesFolderName = path;
+            configuration.setXunitFile(new File("TEST-" + testCasesFolderName.replace(' ', '_') + ".xml"));


I was wondering, wouldn't be better to have test files created under target folder?

Also I saw some other files being created when I ran the tests:

Output: /Users/oalsafi/Work/Apache/camel/components/camel-robotframework/output.xml XUnit: /Users/oalsafi/Work/Apache/camel/components/camel-robotframework/TEST-src/test/resources/org/apache/camel/component/robotframework/set_variable_camel_exchange.robot.xml Log: /Users/oalsafi/Work/Apache/camel/components/camel-robotframework/log.html Report: /Users/oalsafi/Work/Apache/camel/components/camel-robotframework/report.html

So I was wondering as well for these files if they should be created under target?

not neccessary. that option can be user-configured. for unit tests, i set that option to get them generated in target folder.

omarsmak · 2019-09-16T11:26:14Z

+        if (variablesToAdd == null) {
+            return;
+        }
+        String[] splittedVariabledToAdd = variablesToAdd.split(",");


nit: I think you meant here splitVariablesToAdd?

Suggested change

String[] splittedVariabledToAdd = variablesToAdd.split(",");

String[] splitVariablesToAdd = variablesToAdd.split(",");

omarsmak · 2019-09-16T11:29:16Z

+    }
+
+    public void add(String... values) {
+        for (String value : values) {


Do you think is better to replace it with Collection.addAll?

Suggested change

for (String value : values) {

arguments.addAll(Arrays.asList(values));

omarsmak · 2019-09-16T11:33:11Z

+    }
+
+    public boolean isSingleton() {
+        return true;


I think you don't need to override since is by default set to true

omarsmak · 2019-09-16T11:39:42Z

+        generatedArguments.addFileToArguments(configuration.getXunitFile(), "-x");
+        generatedArguments.addFlagToArguments(true, "--xunitskipnoncritical");
+
+        generatedArguments.add(path);


Do you think it would make sense to have the code responsible path creation separated into a different function:

Suggested change

generatedArguments.add(path);

generatedArguments.add(createPath());

not really imho.

The purpose is here to just decompose the function and give more readability and make the code more conscious. However, if you think it doesn't make sense, then is fine

omarsmak · 2019-09-16T11:43:02Z

+
+        if (configuration.getXunitFile() == null) {
+            String testCasesFolderName = path;
+            configuration.setXunitFile(new File("TEST-" + testCasesFolderName.replace(' ', '_') + ".xml"));


Can we just do like this?:

Suggested change

configuration.setXunitFile(new File("TEST-" + testCasesFolderName.replace(' ', '_') + ".xml"));

configuration.setXunitFile(new File("TEST-" + path.replace(' ', '_') + ".xml"));

and remove testCasesFolderName?

onderson · 2019-09-16T23:15:08Z

@oscerd @omarsmak , thanks for the reviews. updated. could you please have another look? thanks

davsclaus · 2019-09-17T02:34:02Z

+/**
+ * Represents a RobotFramework endpoint.
+ */
+@UriEndpoint(firstVersion = "3.0.0-SNAPSHOT", scheme = "RobotFramework", title = "RobotFramework", syntax = "robotframework:resourceUri", label = "test execution")


Remove -SNAPSHOT

davsclaus · 2019-09-17T02:34:28Z

+/**
+ * Represents a RobotFramework endpoint.
+ */
+@UriEndpoint(firstVersion = "3.0.0-SNAPSHOT", scheme = "RobotFramework", title = "RobotFramework", syntax = "robotframework:resourceUri", label = "test execution")


label = "testing"

davsclaus · 2019-09-17T02:34:58Z

+        // either from a directory or from a file
+        String path = getResourceUri();
+        ObjectHelper.notNull(path, "resourceUri");
+        log.info("RobotFrameworkEndpoint resourceUri:{}", path);


Reduce to DEBUG level

onderson · 2019-09-17T10:33:15Z

updated as per comments. @oscerd @omarsmak @davsclaus , could you have a look?

omarsmak

LGTM! Thanks

davsclaus · 2019-09-17T15:09:40Z

I would prefer robot-framework as the schema/component name. And set the title attribute to Robot Framework so there is a nice human readable title.

onderson · 2019-09-17T18:28:51Z

@davsclaus robotframework is kind of popular with robotframework name. not robot-framework. so even though i agree, it might be good to keep robotframework name as it is. for title, you are right, i will update it.

camel-robotframework component: rebase and polish add karaf feature for robotframework component docs and cleanup Fix CS CR changes

davsclaus · 2019-09-18T04:41:19Z

@onderson ah okay about the naming then

onderson · 2019-09-18T08:21:23Z

Thanks guys. merging and closing.

onderson changed the title ~~create camel-robotframework component~~ CAMEL-13979 - create camel-robotframework component Sep 15, 2019

oscerd requested changes Sep 16, 2019

View reviewed changes

omarsmak reviewed Sep 16, 2019

View reviewed changes

davsclaus reviewed Sep 17, 2019

View reviewed changes

onderson force-pushed the camel-robotframework2 branch from 0937055 to 2b13243 Compare September 17, 2019 10:32

omarsmak approved these changes Sep 17, 2019

View reviewed changes

oscerd approved these changes Sep 17, 2019

View reviewed changes

create camel-robotframework component

009fc03

camel-robotframework component: rebase and polish add karaf feature for robotframework component docs and cleanup Fix CS CR changes

onderson force-pushed the camel-robotframework2 branch from 2b13243 to 009fc03 Compare September 17, 2019 20:54

davsclaus approved these changes Sep 18, 2019

View reviewed changes

onderson merged commit ccf1337 into apache:master Sep 18, 2019


		public interface RobotFrameworkCamelConstants {

		public final String ROBOT_CAMEL_EXCHANGE_NAME = "exchange";

	String[] splittedVariabledToAdd = variablesToAdd.split(",");
	String[] splitVariablesToAdd = variablesToAdd.split(",");

	for (String value : values) {
	arguments.addAll(Arrays.asList(values));

	generatedArguments.add(path);
	generatedArguments.add(createPath());

	configuration.setXunitFile(new File("TEST-" + testCasesFolderName.replace(' ', '_') + ".xml"));
	configuration.setXunitFile(new File("TEST-" + path.replace(' ', '_') + ".xml"));

Uh oh!

Conversation

onderson commented Sep 15, 2019

Uh oh!

oscerd left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

omarsmak Sep 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

omarsmak Sep 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

onderson commented Sep 16, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

onderson commented Sep 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

omarsmak left a comment

Choose a reason for hiding this comment

Uh oh!

davsclaus commented Sep 17, 2019

Uh oh!

onderson commented Sep 17, 2019

Uh oh!

davsclaus commented Sep 18, 2019

Uh oh!

onderson commented Sep 18, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

omarsmak Sep 17, 2019 •

edited

Loading

omarsmak Sep 16, 2019 •

edited

Loading

onderson commented Sep 17, 2019 •

edited

Loading