feat(#159): Creates Arquillian SPI to resolve placeholders in configuration files #162

lordofthejars · 2018-02-08T15:51:59Z

Short description of what this resolves:

Creates SPI to resolve placeholders in configuration files

Changes proposed in this pull request:

Creates ConfigurationPlaceholderResolver SPI
Move current logic of sysprop/env into SPI
Implemented classpath resolution

After the discussion I will:

Update javadoc
Update docs

Fixes #159

…ation files

MatousJobanek

I have just a few refactoring comments, otherwise, a really good job with this new feature 👍
I really like it!

MatousJobanek · 2018-02-09T10:56:00Z

...pl-base/src/main/java/org/jboss/arquillian/config/impl/extension/ConfigurationRegistrar.java

+        Collections.sort(configurationPlaceholderResolvers, new Comparator<ConfigurationPlaceholderResolver>() {
+            public int compare(ConfigurationPlaceholderResolver o1, ConfigurationPlaceholderResolver o2) {
+                Integer a = o1.precedence();
+                Integer b = o2.precedence();


I would use firstResolver and secondResolver instead of o1 and o2

MatousJobanek · 2018-02-09T11:03:00Z

...pl-base/src/main/java/org/jboss/arquillian/config/impl/extension/ConfigurationRegistrar.java

+                Integer b = o2.precedence();
+                return b.compareTo(a);
+            }
+        });


the lines 81-90 could be moved to a separated method - loadAndOrderPlaceholderResolvers - to keep this method focused only on config loading.

MatousJobanek · 2018-02-09T11:09:22Z

...pl-base/src/main/java/org/jboss/arquillian/config/impl/extension/StringPropertyReplacer.java

+        String getValue(String key);
+    }
+
+    static class ClasspathPropertyResolver implements PropertyResolver {


from my point of view, having the classes and interface within this class looks really chaotic.
could you please move it outside of the class?

the problem is that it is really a hack for this specific class, it implements like a Java 8 lambda, so something to not be used outside this scope, you can find this approach in several places in JDK code. Although I agree that generally speaking is something I try always to avoid, in this case since it is for simulating lambdas, I preferred to do it in this way.

the problem is that it is really a hack for this specific class

But the logic is called from outside of this class - ConfigurationSysPropResolver & ClasspathConfigurationPlaceholderResolver (btw, these two classes could have similar naming format :-)). You could also call
StringPropertyReplacer.replaceProperties(string, new ClasspathPropertyResolver())
instead of
StringPropertyReplacer.replaceClasspath(descriptorAsString)

Although I agree that generally speaking is something I try always to avoid

then don't do it :-)

in this case since it is for simulating lambdas, I preferred to do it in this way.

what would you say before the lamdas time? :-)

But the logic is called from outside of this class

logic yes but from outside they do not know the existence of these classes. It is something internal.

then don't do it :-)

Generally speaking, does not mean never

But if this is something you see bad I'll extract them outside

MatousJobanek · 2018-02-09T11:40:43Z

config/spi/src/main/java/org/jboss/arquillian/config/spi/ConfigurationPlaceholderResolver.java

+    ArquillianDescriptor resolve(ArquillianDescriptor arquillianDescriptor);
+
+    /**
+     * In case then more of one placeholder resolver, they are ordered as they appear on classpath.


In case then more of one placeholder resolver
please fix the wording

MatousJobanek · 2018-02-09T11:48:16Z

...st/java/org/jboss/arquillian/config/impl/extension/ClasspathReplacementInArqXmlTestCase.java

+import static org.jboss.arquillian.config.descriptor.impl.AssertXPath.assertXPath;
+
+@RunWith(MockitoJUnitRunner.class)
+public class ClasspathReplacementInArqXmlTestCase extends AbstractManagerTestBase {


correct me if I'm wrong - this class is almost same as SyspropReplacementInArqXmlTestCase. The only difference is the test method which are just a few lines of code - merge it into one test class or (maybe better) create an abstract-template class with the common logic and extend it.

MatousJobanek · 2018-02-09T11:49:31Z

...st/java/org/jboss/arquillian/config/impl/extension/ClasspathReplacementInArqXmlTestCase.java

+    public void should_replace_classpath_in_arquillian_xml() throws Exception {
+
+        final String xml = desc.get().exportAsString();
+        System.out.println(xml);


is the system out necessary?

I have been there in all tests from the early ages :) so I think it is not necessary but who knows :D (check other tests so you can see it). I can remove it from our tests but how about the others?

I saw it in SyspropReplacementInArqXmlTestCase and my feeling is that it was forgotten after some debugging. From my point of view, there is no benefit of having it there - it just generates an unnecessary ballast in the output. If it is in some other test cases that you don't change in this PR, then keep it there, but please don't put it into the new tests...

It is in a lot of tests :D

MatousJobanek · 2018-02-09T11:53:25Z

...st/java/org/jboss/arquillian/config/impl/extension/ClasspathReplacementInArqXmlTestCase.java

+
+        final String xml = desc.get().exportAsString();
+        System.out.println(xml);
+        AssertXPath.assertXPath(xml, "/arquillian/container/@qualifier", VALUE_EL_OVERRIDE);


this asserts the logic of ConfigurationSysPropResolver - that the system property is resolved, but the method is called should_replace_classpath_in_arquillian_xml - either remove the assert or change the method name

It would be also good to use given-when-then comments

about given/when/then this is something not used in Arquillian Core, should we start doing it and have tests written in different "layout" or continue so they look the same?

I would say "start doing it" - btw, you have already started in ArquillianDescriptorClasspathPropertiesTestCase ;-) https://github.com/arquillian/arquillian-core/pull/162/files#diff-3f0690a66704804b5e1a1a151cae218cR15

MatousJobanek · 2018-02-09T12:00:44Z

...pl-base/src/main/java/org/jboss/arquillian/config/impl/extension/StringPropertyReplacer.java

+                String classpathResource = key.substring(CLASSPATH.length(), key.length() - 1);
+                final URL resource = contextClassLoader.getResource(classpathResource);
+
+                //If resource is not found it is returned as null so no change is applicable.


It would be also good to log some information that the resource wasn't loaded from classpath

Notice that in case of system properties if they are not resolved then no log is printed, they are just returned as is, so why we should do in case of classpath? What I mean is that if we do not log in case of sysprops nor envvars then it seems reasonable to not do it in case of classpath.

sysprops is something expected and obvious - it works in bash, pom files and other programming languages in the same way. Resolving ${classpath()} is something completely special and new just for our Arquillian case... That's why I would make it transparent what is going on and why it wasn't resolved...

MatousJobanek · 2018-02-09T12:01:57Z

.../jboss/arquillian/config/impl/extension/ArquillianDescriptorClasspathPropertiesTestCase.java

+
+public class ArquillianDescriptorClasspathPropertiesTestCase {
+
+


empty line - same for the line 67

…nd fix javadoc

lordofthejars · 2018-02-09T13:17:31Z

@MatousJobanek I have updated the PR

MatousJobanek · 2018-02-09T14:14:53Z

...base/src/main/java/org/jboss/arquillian/config/impl/extension/ClasspathPropertyResolver.java

+
+            //If resource is not found it is returned as null so no change is applicable.
+            if (resource == null) {
+                logger.warning(String.format("Resource %s is not find in classspath and it is not replaced.", classpathResource));


maybe better log message would be:
String.format("Resource %s is not found on the classspath so the property %s is not replaced.", classpathResource, key)

MatousJobanek · 2018-02-09T14:22:58Z

...st/java/org/jboss/arquillian/config/impl/extension/ClasspathReplacementInArqXmlTestCase.java

+    /**
+     * Sets the name of the arquillian.xml under test
+     */
+    @BeforeClass


both @BeforeClass and @AfterClass methods are almost same (including the variables), shouldn't that be moved to the template as well? I know that the arquillian.xml file name is different - that can be retrieved by an abstract method getArquillianXmlFileName()

I understand what you mean and I will do but I always have the feeling that using abstractions in tests makes look tests somehow less readable in the sense of just checking the class and know what they are doing (you need to navigate through parent classes and be present about that), but I know it is something personal.

In fact, this approach you mention does not work because @BeforeAll is (and must) be static so we cannot retrieve it from an abstract method. So we can put this logic in @Before block but at the end this logic is something that should be executed only once.

Yeah, right. You know, it's Friday and my brain is probably in the weekend mode :-D

MatousJobanek · 2018-02-09T14:23:52Z

config/impl-base/src/main/java/org/jboss/arquillian/config/impl/extension/ConfigExtension.java

@@ -29,5 +30,8 @@
    @Override
    public void register(ExtensionBuilder builder) {
        builder.observer(ConfigurationRegistrar.class);
+
+        builder.service(ConfigurationPlaceholderResolver.class, ConfigurationSysPropResolver.class);
+        builder.service(ConfigurationPlaceholderResolver.class, ClasspathConfigurationPlaceholderResolver.class);


Please, unify the naming format of the classes:
ConfigurationSysPropResolver
ClasspathConfigurationPlaceholderResolver

MatousJobanek · 2018-02-09T14:25:04Z

config/impl-base/pom.xml

+      <artifactId>arquillian-test-spi</artifactId>
+      <version>${project.version}</version>
+      <scope>test</scope>
+    </dependency>


this dependency is not necessary, you can remove it

MatousJobanek

Awesome, thanks a lot! The code is OK from my side. Just the doc is missing, but you have it in the task list already :-)

bartoszmajsak

Great stuff @lordofthejars! Thank you.

Can you make JIRA for it and link the PR?

bartoszmajsak · 2018-02-19T11:19:15Z

...st/java/org/jboss/arquillian/config/impl/extension/ClasspathReplacementInArqXmlTestCase.java

+    @AfterClass
+    public static void clearSysprops() {
+        System.clearProperty(ConfigurationRegistrar.ARQUILLIAN_XML_PROPERTY);
+        System.clearProperty(SYSPROP_ARQ_CONTAINER);


Was wondering if we can't add https://github.com/stefanbirkner/system-rules for that?

Requires JDK 6 or above, we are at 5 😢

lordofthejars · 2018-02-22T09:28:58Z

https://issues.jboss.org/browse/ARQ-2177

feat(arquillian#159): Creates SPI to resolve placeholders in configur…

0464ffb

…ation files

lordofthejars requested review from bartoszmajsak and MatousJobanek February 8, 2018 15:55

MatousJobanek requested changes Feb 9, 2018

View reviewed changes

chore(refactor): Extract inner classes, abstract common test pieces a…

a3f4e84

…nd fix javadoc

MatousJobanek requested changes Feb 9, 2018

View reviewed changes

lordofthejars added 2 commits February 9, 2018 16:01

chore(refactor): Rename classes to follow same nomenclature

4c0498c

chore: Removes old class not used for the new one

be371be

MatousJobanek approved these changes Feb 9, 2018

View reviewed changes

lordofthejars mentioned this pull request Feb 18, 2018

feat(#81): Adds annotation configuration for containers arquillian/arquillian-container-chameleon#83

Merged

1 task

bartoszmajsak reviewed Feb 19, 2018

View reviewed changes

lordofthejars mentioned this pull request Feb 19, 2018

Make standard placeholder resolvers in extensions arquillian/arquillian-organization#30

Open

lordofthejars merged commit 57e2417 into arquillian:master Feb 22, 2018

lordofthejars deleted the issue-159 branch February 22, 2018 09:30

This pull request was closed.


		public class ArquillianDescriptorClasspathPropertiesTestCase {

feat(#159): Creates Arquillian SPI to resolve placeholders in configuration files #162

feat(#159): Creates Arquillian SPI to resolve placeholders in configuration files #162

Conversation

lordofthejars commented Feb 8, 2018 • edited by bartoszmajsak Loading

Short description of what this resolves:

Changes proposed in this pull request:

MatousJobanek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lordofthejars commented Feb 9, 2018

Choose a reason for hiding this comment

MatousJobanek Feb 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lordofthejars Feb 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MatousJobanek Feb 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MatousJobanek left a comment • edited Loading

Choose a reason for hiding this comment

bartoszmajsak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lordofthejars commented Feb 22, 2018

lordofthejars commented Feb 8, 2018 •

edited by bartoszmajsak

Loading

MatousJobanek Feb 9, 2018 •

edited

Loading

lordofthejars Feb 9, 2018 •

edited

Loading

MatousJobanek Feb 9, 2018 •

edited

Loading

MatousJobanek left a comment •

edited

Loading