Asciidoctor :: Discussion

Re: [dev] Integrating Arquillian into the AsciidoctorJ tests

_‹ Previous Topic Next Topic _›

Classic

List

Threaded

13 messages Options

mojavelinux

Re: [dev] Integrating Arquillian into the AsciidoctorJ tests

Administrator

First of all, great work! This is a seriously cool application of Arquillian.

I just got it all running on my machine. Green bar!

I really like the Asciidoctor injection! That drastically cleans up the test. Now, we can really focus on the test without worrying about the infrastructure. Exactly the problem that Arquillian was designed to solve. It also shows how useful Arquillian is even when we don't have a container in which to deploy (though we could deploy these tests into a container for even more isolation in the future).

> - Whether this is useful

Definitely. You outlined the reasons very well.

I would like to see more control at the method injection point as to whether Asciidoctor is created with a new Ruby instance or whether it reuses an existing Ruby instance. For instance, perhaps it reuses the Ruby instance by default and the following would provide a new Ruby instance:

@Test
public void should_return_if_it_is_block(@ArquillianResource @RubyIsolation(SHARED) Asciidoctor asciidoctor) {
DocumentRuby document = asciidoctor.load(DOCUMENT, new HashMap<String, Object>());
assertThat(document.isBlock(), is(true));
}

I'm not sure what the right annotation name / property combination are...perhaps we can read the literature on JRuby about the proper terminology here. We just need somewhat to communicate whether the Ruby instance is new or shared.

> - If the split into an API part and an Impl part is useful.

Indeed. It makes sense and needs to happen at some point. I would lean towards making it as minimal as possible so that we don't get stuff into the API too soon. Of course the AST classes need to be there, so it's a good opportunity to do that refactoring.

> - What other ideas do you have in this area?

I think the ClasspathResources should be injected using the @ArquillianResource as well. This involves creating a resource provider that creates this instance. When I see @Rule on a test, I cringe ;) Perhaps move it into the Arquillian package as well. Eventually we may want to be able to inject these resources, but honestly it's probably find just to have the ClasspathResources instance available.

I definitely think it's time to switch to Spock as well for the test suite. Writing tests in Java is really verbose and a lot of these tests could be better described in a BDD style. We could start by having both JUnit and Spock in use, but eventually it would be nice to make most (if not all) tests in Spock.

We could consider putting the test support in core/src/test, even if we have an API module. This is the structure the Arquillian project uses to test itself. It avoids the overhead of the extra module in the project. I'm not opposed to having the test-support module, though, so it's up to you what you think is best.

Let's work to get this code into the master branch so that everyone can use it without having to juggle a lot of branches.

Fantastic work! It's super exciting to see the test suite improve so dramatically. I look forward to hearing from others.

-Dan

On Wed, Apr 1, 2015 at 3:37 PM, Robert.Panzer [via Asciidoctor :: Discussion] <[hidden email]> wrote:

Hi,

I am currently doing some "research" on trying to bring Arquillian into the AsciidoctorJ tests.
I do this because:

- Currently every test class creates an own Asciidoctor instance that is shared among the test methods, so that the tests are not fully isolated
- Asciidoctor instances and their associated Ruby instances are not terminated at the end of the test
- The code to create the Asciidoctor instance is spread over all tests.

Therefore I have started work on an Arquillian JRuby container that allows to create Ruby or Scripting container instances per test class or per test method and inject them as ArquillianResources.
The current state is available at https://github.com/robertpanzer/arquillian-container-jruby, the relevant branch is 'ArquillianResource':
https://github.com/robertpanzer/arquillian-container-jruby/tree/ArquillianResource

An example looks like this:
    @Test
    @RubyScript("testscript.rb")
    public void secondTest(@ArquillianResource ScriptingContainer scriptingContainer) {
        assertEquals(1L, scriptingContainer.runScriptlet("@a"));
    }
At the moment it depends on arquillian-core 1.1.8.Final-SNAPSHOT!

Then I started to integrate that into AsciidoctorJ.
The current state is available in my fork in the branch ArquillianJrubyContainer:
https://github.com/robertpanzer/asciidoctorj/tree/ArquillianJrubyContainer

There I added in the test-support subproject an Arquillian extension that plays together nicely with the JRuby container and knows how to inject Asciidoctor instances:
https://github.com/robertpanzer/asciidoctorj/blob/ArquillianJrubyContainer/asciidoctorj-core%2Fsrc%2Ftest%2Fjava%2Forg%2Fasciidoctor%2FWhenAsciiDocIsRenderedToDocument.java#L65
Every Asciidoctor & Ruby instance is terminated after a test.
    @Test
    public void should_return_if_it_is_block(@ArquillianResource Asciidoctor asciidoctor) {
        DocumentRuby document = asciidoctor.load(DOCUMENT, new HashMap<String, Object>());
        assertThat(document.isBlock(), is(true));
    }
To make this possible I had to split asciidoctorj-core into an API and an Impl part.
Asciidoctor.Factory now uses a ServiceLoader implementation to initialize the implementation instead of directly calling JRubyAsciidoctor.

So with this mail I want to start a discussion:

- Whether this is useful
- If the split into an API part and an Impl part is useful.
I'd say yes because it also paves the way to support JavaScript or native Java implementations.
- What other ideas do you have in this area?

So it would be great to get some feedback on that.

Best regards,
Robert

If you reply to this email, your message will be added to the discussion below:
http://discuss.asciidoctor.org/dev-Integrating-Arquillian-into-the-AsciidoctorJ-tests-tp2910.html

To start a new topic under Asciidoctor :: Discussion, email [hidden email]
To unsubscribe from Asciidoctor :: Discussion, click here.
NAML

Dan Allen | http://google.com/profiles/dan.j.allen

Robert.Panzer

Re: [dev] Integrating Arquillian into the AsciidoctorJ tests

Regarding the isolation of the Asciidoctor and Ruby instances the current implementation works similar to Drone (Aslak pointed me to this solution):

- Instances injected into a test method are also created only per test method.
- Instances injected into the test class are shared among all test methods.

Do you think this could already be a reasonable solution?
Or do you prefer to make it more explicit by adding an additional qualifier?

Trying to get a first Spock POC running is one of the next tasks on my list:)
Even though I am currently still reading BDD in action. ;-)

Regarding putting the arquillian extension into core/src/test I am a bit unsure.
The other projects also need these test infrastructure classes and we either have to separate them from the core tests or the core tests will at least be visible to the classloader when testing pdf, epub and diagram.

Best regards,
Robert

mojavelinux

Re: [dev] Integrating Arquillian into the AsciidoctorJ tests

Administrator

On Thu, Apr 2, 2015 at 7:53 PM, Robert.Panzer [via Asciidoctor :: Discussion] <[hidden email]> wrote:

Or do you prefer to make it more explicit by adding an additional qualifier?

I'd like to see it more explicit. Sometimes, a single test may need different behavior when injected into a test method. We don't want that code style necessarily to be hardwired to capability (just perhaps by default).

Cheers,

-Dan

Dan Allen | http://google.com/profiles/dan.j.allen

mojavelinux

Re: [dev] Integrating Arquillian into the AsciidoctorJ tests

Administrator

In reply to this post by Robert.Panzer

On Thu, Apr 2, 2015 at 7:53 PM, Robert.Panzer [via Asciidoctor :: Discussion] <[hidden email]> wrote:

Trying to get a first Spock POC running is one of the next tasks on my list:)
Even though I am currently still reading BDD in action. ;-)

Nice! Definitely a good read. Changes your thinking about tests in my experience, even if you already knew BDD.

-Dan

Dan Allen | http://google.com/profiles/dan.j.allen

Robert.Panzer

Re: [dev] Integrating Arquillian into the AsciidoctorJ tests

In reply to this post by mojavelinux

This should go into the Ruby container as well.

I'll add a new qualifier to it that will be reused by the asciidoctor extension in test-support.

Intuitively I'd say that all resources are isolated by default.

Then I could even remove the dependency on arquillian-core 1.1.8.Final-SNAPSHOT, because there would be no more implicit logic. Nice!

Best regards

Robert

Am 02.04.2015 um 22:47 schrieb mojavelinux [via Asciidoctor :: Discussion] <[hidden email]>:

On Thu, Apr 2, 2015 at 7:53 PM, Robert.Panzer [via Asciidoctor :: Discussion] <[hidden email]> wrote:
Or do you prefer to make it more explicit by adding an additional qualifier?

I'd like to see it more explicit. Sometimes, a single test may need different behavior when injected into a test method. We don't want that code style necessarily to be hardwired to capability (just perhaps by default).

Cheers,

-Dan

--
Dan Allen | http://google.com/profiles/dan.j.allen

If you reply to this email, your message will be added to the discussion below:
http://discuss.asciidoctor.org/Re-dev-Integrating-Arquillian-into-the-AsciidoctorJ-tests-tp2943p2953.html

To unsubscribe from Re: [dev] Integrating Arquillian into the AsciidoctorJ tests, click here.
NAML

mojavelinux

Re: [dev] Integrating Arquillian into the AsciidoctorJ tests

Administrator

On Fri, Apr 3, 2015 at 12:01 AM, Robert.Panzer [via Asciidoctor :: Discussion] <[hidden email]> wrote:

Intuitively I'd say that all resources are isolated by default.

I could definitely be okay with that, especially if it's clearly stated and thus well understood...that's a default that is very much inline with the Arquillian philosophy.

-Dan

Dan Allen | http://google.com/profiles/dan.j.allen

Robert.Panzer

Re: [dev] Integrating Arquillian into the AsciidoctorJ tests

In reply to this post by mojavelinux

And here they are again my three problems (wild translation from German;-) )

I have a problem with the naming:

The Arquillian JRuby container already has an isolated flag in the arquillian.xml that defines if the Ruby instance sees everything on the classpath, like probably all embedded containers do, or if it's isolated so that it only sees the gems and resources in the Deployment.

I think it's hard to explain this ambiguity.

And IMO the isolated name also fits for this configuration parameter.

What do you think about a @ResourceScope?

<raw>

void test(@ArquillianResource @ResourceScope(TEST_METHOD) Asciidoctor asciidoctor) {...}

void test(@ArquillianResource @ResourceScope(TEST_CLASS) Asciidoctor asciidoctor) {...}

</raw>

Best regards

Robert

Am 02.04.2015 um 22:47 schrieb mojavelinux [via Asciidoctor :: Discussion] <[hidden email]>:

On Thu, Apr 2, 2015 at 7:53 PM, Robert.Panzer [via Asciidoctor :: Discussion] <[hidden email]> wrote:
Or do you prefer to make it more explicit by adding an additional qualifier?

I'd like to see it more explicit. Sometimes, a single test may need different behavior when injected into a test method. We don't want that code style necessarily to be hardwired to capability (just perhaps by default).

Cheers,

-Dan

--
Dan Allen | http://google.com/profiles/dan.j.allen

If you reply to this email, your message will be added to the discussion below:
http://discuss.asciidoctor.org/Re-dev-Integrating-Arquillian-into-the-AsciidoctorJ-tests-tp2943p2953.html

To unsubscribe from Re: [dev] Integrating Arquillian into the AsciidoctorJ tests, click here.
NAML

mojavelinux

Re: [dev] Integrating Arquillian into the AsciidoctorJ tests

Administrator

On Fri, Apr 3, 2015 at 9:38 AM, Robert.Panzer [via Asciidoctor :: Discussion] <[hidden email]> wrote:

What do you think about a @ResourceScope?

<raw>
void test(@ArquillianResource @ResourceScope(TEST_METHOD) Asciidoctor asciidoctor) {...}

void test(@ArquillianResource @ResourceScope(TEST_CLASS) Asciidoctor asciidoctor) {...}
</raw>

In general I like this. We need to make it clear which resource we're talking about. I'm not so concerned about the scope of the Asciidoctor instance (though maybe). What we're mainly concerned with is the scope of the _Ruby_ instance. Thus, we may need someway to refer to the subject to which the scope applies.

void test(@ArquillianResource(SharedScriptingContainer.class) Asciidoctor asciidoctor) {...}

(where SharedScriptingContainer is just an marker class)

Aslak and I discussed this and what we arrived at (I think) is that we are trying to communicate that it isn't necessary that the ScriptingContainer be isolated. How isolated it is, however, is up to the adapter. It could be application-scoped, it could be recreated every X number of methods, etc. The test really doesn't care. The test is saying "don't go out of your way to keep this isolated").

wdyt?

-Dan

Dan Allen | http://google.com/profiles/dan.j.allen

Robert.Panzer

Re: [dev] Integrating Arquillian into the AsciidoctorJ tests

Please correct me from wrong, as my Ruby knowledge is at best superficial:
But in my current thinking the lifetime of an Asciidoctor and its extension registry etc is bound to the Ruby or ScriptingContainer instance as there are no Ruby objects created for Java Asciidoctor instances.
So in my current thinking an Asciidoctor instance maps 1:1 to a ScriptingContainer.

At this very moment I think that we should get rid of the Arquillian JRuby extension for these reasons:
1. We don't need any of its major features, like isolation, gem installation & caching & Ruby script execution
2. Creating the Ruby instances outside the AsciidoctorJ core requires to pass it when creating an Asciidoctor instance and therefore we still have a Ruby dependency in the api project that we finally want to get rid off (in the API, certainly not the implementation!)

So to manage the lifetime of Asciidoctor instances what do you think about of adding a destroy() method to clean up the wrapped Ruby instance?
Therefore the sequence of method invocations would look like this:

Asciidoctor asciidoctor = Asciidoctor.Factory.create(); // Called by Arquillian extension
asciidoctor.renderFile(); // methods invoked by test methods
asciidoctor.convert();
asciidoctor.destroy(); // Called by Arquillian extension again when requested scope is finished or by a client of the API if he no longer needs the instance

(Strange that I am always shooting against my own work;-) )

I like the idea of adding a marker class to the ArquillianResource annotation instead of adding a new annotation.
I always asked myself of the use of the class passed to the ArquillianResource annotation.
Now I know a use case :)

What do you think?

Best regards,
Robert

Robert.Panzer

Re: [dev] Integrating Arquillian into the AsciidoctorJ tests

Bad formulation:

So in my current thinking an Asciidoctor instance maps 1:1 to a ScriptingContainer.

Instead I wanted to say all Asciidoctor instances that use the same ScriptingContainer share the same state, so isolation is not based on the Asciidoctor instance but on the underlying ScriptingContainer.

Therefore we have to take care of the static Ruby runtime that is currently used in AsciidoctorJ:
https://github.com/asciidoctor/asciidoctorj/blob/master/asciidoctorj-core/src/main/java/org/asciidoctor/internal/JRubyRuntimeContext.java#L17

Robert.Panzer

Re: [dev] Integrating Arquillian into the AsciidoctorJ tests

I've stepped back again and started anew.

This time without using the Arquillian JRuby container.
Instead an own Arquillian extension in test-support creates the Asciidoctor instances and destroys them after the test.
Therefore I've add a new method destroy() to the Asciidoctor interface:

You can see the result here:
https://github.com/robertpanzer/asciidoctorj/tree/ApiImplSplit

First I've factored out an API project that is now completely independent of JRuby.
Asciidoctor.Factory.create() uses a service implementation of AsciidoctorProvider to create new instances.
asciidoctorj-core provides an implementation that creates a new JRubyAsciidoctor instance.
https://github.com/robertpanzer/asciidoctorj/blob/ApiImplSplit/asciidoctorj-core/src/main/java/org/asciidoctor/internal/JRubyAsciidoctorProvider.java
In the future other implementation projects could simply provider another service implementation to provide a Java or Nashorn implementation to AsciidoctorJ.

A detail here is that I changed the type of the options to the converter from Map<Object,Object> to Map<String,Object>.
Object keys have currently been imo only Ruby symbols, the implementation should hide this implementation detail and map back and forth as required.
Does that make sense?

At the moment real test isolation is difficult because the current Ruby runtime is held in a static member in JRubyRuntimeContext:
https://github.com/asciidoctor/asciidoctorj/blob/master/asciidoctorj-core/src/main/java/org/asciidoctor/internal/JRubyRuntimeContext.java
I've removed this class completely and the Ruby instance is now only held in instance variables allowing for multiple isolated Asciidoctor instances.

The test-support project contains an arquillian extension that allows to inject private Asciidoctor instance per test or shared instances for the test class:
https://github.com/robertpanzer/asciidoctorj/blob/ApiImplSplit/asciidoctorj-core/src/test/java/org/asciidoctor/test/arquillian/WhenAsciidoctorIsInjectedFromArquillian.java

Asciidoctor instances are injected like this:

    @ArquillianResource
    private Asciidoctor unspecifiedClassAsciidoctor;

    @ArquillianResource(Shared.class)
    private Asciidoctor sharedClassAsciidoctor;

    @ArquillianResource(Unshared.class)
    private Asciidoctor unsharedClassAsciidoctor;

And they get destroyed after the test or the test class.

I think this solution is better suited than the previous one I presented above because the code to create the Ruby instances stays the same and does not differ between test and "production".

What do you think about this?

mojavelinux

Re: [dev] Integrating Arquillian into the AsciidoctorJ tests

Administrator

Thanks for the detailed summaries, Robert!

Comments inline.

Robert.Panzer wrote:

I've stepped back again and started anew.

Often the quickest way to a solution :)

I've add a new method destroy() to the Asciidoctor interface:

My first reaction was that I didn't like it...but if we see Asciidoctor as a lifecycle object, then I suppose it's logical to have a destroy() to accompany create().

I've factored out an API project that is now completely independent of JRuby.

I think this is the right way forward. Adding an API jar does introduce an extra bit of complexity for the consumer, but from conversations we've had, it's clearly the direction we keep leaning towards.

Asciidoctor.Factory.create() uses a service implementation of AsciidoctorProvider to create new instances.

Nice!

asciidoctorj-core provides an implementation that creates a new JRubyAsciidoctor instance.

Very nice.

In the future other implementation projects could simply provider another service implementation to provide a Java or Nashorn implementation to AsciidoctorJ.

Finally, the change we've wanted for a long time :) I know Alex had it going in a branch, but it's good to see it making its way to the mainline.

A detail here is that I changed the type of the options to the converter from Map<Object,Object> to Map<String,Object>.
Object keys have currently been imo only Ruby symbols, the implementation should hide this implementation detail and map back and forth as required.
Does that make sense?

Yes, we've talking about hiding Symbol as much as possible. Interesting to note, Asciidoctor.js treats String and Symbol as the same object, so we can't have a difference in the API anyway. I think AsciidoctorJ should take the same route as Asciidcotor.js.

At the moment real test isolation is difficult because the current Ruby runtime is held in a static member in JRubyRuntimeContext:
https://github.com/asciidoctor/asciidoctorj/blob/master/asciidoctorj-core/src/main/java/org/asciidoctor/internal/JRubyRuntimeContext.java
I've removed this class completely and the Ruby instance is now only held in instance variables allowing for multiple isolated Asciidoctor instances.

Cool! I do like that. As long as it's possible to have multiple Asciidoctor instances that also share a Ruby runtime, then this is the way forward.

The test-support project contains an arquillian extension that allows to inject private Asciidoctor instance per test or shared instances for the test class:
https://github.com/robertpanzer/asciidoctorj/blob/ApiImplSplit/asciidoctorj-core/src/test/java/org/asciidoctor/test/arquillian/WhenAsciidoctorIsInjectedFromArquillian.java

Asciidoctor instances are injected like this:
    @ArquillianResource
    private Asciidoctor unspecifiedClassAsciidoctor;

    @ArquillianResource(Shared.class)
    private Asciidoctor sharedClassAsciidoctor;

    @ArquillianResource(Unshared.class)
    private Asciidoctor unsharedClassAsciidoctor;
And they get destroyed after the test or the test class.

I love it.

I think this solution is better suited than the previous one I presented above because the code to create the Ruby instances stays the same and does not differ between test and "production".

What do you think about this?

Overall, I like it. It hits many of the requirements we've been trying to get for a long time, so I'm excited about the change it's bringing.

One thing I'd like to see come out of this is more careful documentation about how the Ruby runtime is handled, where it is stored and overall its lifecycle. That's one of the reasons for some confusion in AsciidoctorJ and if this change can bring clarity, it will really accelerate development. I think the Arquillian tests go a long way towards making that more clear.

I think the toughest part still is getting the API for AsciidoctorJ right. Since we're making a change to it, we want to think about making it a solid foundation for the future so that we don't have to fiddle with it too much after this migration. If you get the changes merged together, we can seek input from the API masters and see if we can get it really polished.

Great work!

Cheers,

-Dan

Dan Allen | http://google.com/profiles/dan.j.allen

Robert.Panzer

Re: [dev] Integrating Arquillian into the AsciidoctorJ tests

Thanks for the compliments! :)

I am a bit unsure about this atm:

Cool! I do like that. As long as it's possible to have multiple Asciidoctor instances that also share a Ruby runtime, then this is the way forward.

With two Asciidoctor instances that share the same Ruby runtime the registration of extensions might behave strangely.

If I understood the Asciidoctor Ruby code correctly the Extension Registry is a singleton.
Then registering an extension at one AsciidoctorJ instance would result in having it registered with the second as well, wouldn't it?
Having one Ruby instance per Asciidoctor instance would split the singletons that the we get a per instance behavior that at least I would expect when calling asciidoctor.javaExtensionRegistry().preprocessor(...).
(Correct me from wrong, my current understanding of the behavior might be completely wrong.)

And if I am still right ;-), Asciidoctor is thread safe, meaning that one AsciidoctorJ instance could be used concurrently, if the extensions and converters are thread safe.

Dan, if you are ok with this, I would start preparing a PR from this branch.
Should I revert the API separation again? Because you did not seem completely convinced with it atm.

Documenting the JRuby stuff is really an important task, but it requires learning about it before ;-)
I am still struggling to learn the differences between new ScriptingContainer() with its properties and JavaEmbedUtils.initialize().

Thanks and best regards,
Robert