FWD » Core Development » Conversion Tools

OEUnit is an open source project (Eclipse Public License) which preceded ABLUnit and has no relation to Progress. It is not part of OpenEdge but would be used as part of a development environment.

ABLUnit is the Progress implementation of xunit testing and it is not exactly a full replacement but it has replacements for most things, some useful additions/enhancements and some things missing.

Useful articles:

https://community.progress.com/s/question/0D54Q000086yObESAU/migrating-tests-from-oeunit-to-ablunit
https://www.progresstalk.com/threads/oeunit-and-other-unit-test-frameworks.121225/

OEUnit includes quite a bit of UI code (including GUI) for running tests. Early parsing of this code shows that it may need tweaks to get it parsed. I haven't evaluated the gaps to be filled if we were to convert that code. I didn't do the gap analysis because I don't see any real value in running the existing OEUnit user interface. Instead, I think it is much better to migrate to JUnit as the tooling to execute tests.

This means that the OEUnit user interface will not be supported. More importantly, do not plan to port or convert the OEUnit code. Instead, existing test annotations will be migrated to JUnit and equivalent test facilities will be provided using JUnit directly.

I intend for this implementation to be in common with the ABLUnit approach in #3827. Although there are some differences in functionality between OEUnit and ABLUnit, the core functionality is the same and hopefully will be common code. In doing this work, it is OK to look at the OEUnit code (but not OK to look at the ABLUnit code). The license status of ABLUnit is unclear, so in an abundance of caution we will avoid any reference to or use of that ABLUnit code.

As noted in #3827-6, our approach will be to implement a FWD TestEngine for JUnit 5. We will NOT implement testing as a part of the FWD server lifecycle itself, but we might need to implement changes in the FWD runtime to allow the TestEngine to connect and execute. If such facilities are needed, let's discuss it here before we implement changes. It is likely that we already have enough support in FWD for this, so it is unclear what might be needed.

You need to add the skeleton project in cfg/p2j.cfg.xml via:

      <parameter name="oo-skeleton-path" value="path/to/skeleton" />

There's something else wrong with your config:

BLOCK (0x00001b00000001): Unknown file name. 

I've never reached this error.

Please see the xfer testcases project configuration for conversion, that is setup to convert .cls files.

I want to convert OEUnit 4gl sources

Are you trying to convert OEUnit itself? There is likely to be code in there that won't convert, especially on the UI side. It is also not clear if the version you can download from github will actually compile in the 4GL. As noted above, we are not going to port that code.

If you are trying to convert sample 4GL test code that uses OEUnit annotations and which would be executed in the 4GL using OEUnit, then I have misunderstood.

Greg Shah wrote:

I want to convert OEUnit 4gl sources

Are you trying to convert OEUnit itself?

In general, yes. I would like to try convert as much of the original OEUnit code as possible just to see what is missing in FWD.
But in this very case I try to convert just any 4gl class code, which does not relate to OEUnit at all. And I need this since OEUnit tests are implemented as 4gl classes. So I have to be able to convert 4gl classes first, then to learn how FWD converts and runs 4gl classes.

It is also not clear if the version you can download from github will actually compile in the 4GL.

It does compile and can run as expected in OE.

As noted above, we are not going to port that code.

Nevertheless, I need a working original system to compare the behavior.

Are you working with the new testcases project (Testcases)? This is already configured properly to convert OO classes. This is the same project in which you will find ABLUnit examples. When you write OEUnit examples that match the ABLUnit cases, you would place them in that project as well.

Greg Shah wrote:

Are you working with the new testcases project (Testcases)? This is already configured properly to convert OO classes. This is the same project in which you will find ABLUnit examples. When you write OEUnit examples that match the ABLUnit cases, you would place them in that project as well.

Yes. Classes can be converted in testcases project from xfer, but cannot in my test project. I have compared the p2j.cfg.xml files in both projects, and I see no related difference :-(

Is your current directory /home/vvt/Projects/? In other words, your conversion project top-level directory is /home/vvt/Projects/?

Greg Shah wrote:

Is your current directory /home/vvt/Projects/? In other words, your conversion project top-level directory is /home/vvt/Projects/?

No the directory is /home/vvt/Projects/test3820. Why? And I use the convert.list target in both cases.

Pointing to a skeletons directory outside of your project root may be causing the issue or at least an issue. The skeletons directory should be located inside the project root.

Greg Shah wrote:

Pointing to a skeletons directory outside of your project root may be causing the issue or at least an issue. The skeletons directory should be located inside the project root.

1. this does not help
2. the skeleton is located outside of testcases either, but classes are compiled in testcases :-(

Classes can be converted by ant convert, but by ant convert.list.

Vladimir Tsichevski wrote:

Greg Shah wrote:

Pointing to a skeletons directory outside of your project root may be causing the issue or at least an issue. The skeletons directory should be located inside the project root.

1. this does not help
2. the skeleton is located outside of testcases either, but classes are compiled in testcases :-(

Do it anyway, please.

Classes can be converted by ant convert, but by ant convert.list.

Do you have all classes in the entire parsed object graph in the list? That is important. The only exceptions are that the referenced skeletons should not be in the conversion list.

Greg Shah wrote:

1. this does not help
2. the skeleton is located outside of testcases either, but classes are compiled in testcases :-(

Do it anyway, please.

I did :-|

Classes can be converted by ant convert, but by ant convert.list.

Do you have all classes in the entire parsed object graph in the list? That is important. The only exceptions are that the referenced skeletons should not be in the conversion list.

I was converting a single class (NoPackageClass.cls) arbitrary selected from testcases and copied to my test project. The ant convert target successfully convert this class along with all other non-class 4gl sources. In testcases the same class can be converted with ant convert.list target.

In list mode, you may only have a portion of the dependencies in the object graph. That will fail. In non-list mode the conversion will be picking up all class files and so it works since the dependencies are there.

If that is not the issue, then you'll need to debug to dig further.

Greg Shah wrote:

In list mode, you may only have a portion of the dependencies in the object graph. That will fail. In non-list mode the conversion will be picking up all class files and so it works since the dependencies are there.

Why it does not fail in testcases then?

If that is not the issue, then you'll need to debug to dig further.

Agreed, this problem does not relate to this issue directly.

In list mode, you may only have a portion of the dependencies in the object graph. That will fail. In non-list mode the conversion will be picking up all class files and so it works since the dependencies are there.

Why it does not fail in testcases then?

Are the source file lists the same?

Greg Shah wrote:

In list mode, you may only have a portion of the dependencies in the object graph. That will fail. In non-list mode the conversion will be picking up all class files and so it works since the dependencies are there.

Why it does not fail in testcases then?

Are the source file lists the same?

Yes

I don't object to the change, but we don't want to spent time on making external files work at this time. We have many places like this which will not work with inputs that are outside of the project home. Please move everything inside the project.

Vladimir, please create a 4GL package for the class - I think FWD has problems converting .cls files without a package. So, change your TestClass as oo.TestClass, place it in a oo/ folder and convert again. Most likely this is the issue.

Greg Shah wrote:

I don't object to the change, but we don't want to spent time on making external files work at this time. We have many places like this which will not work with inputs that are outside of the project home. Please move everything inside the project.

It is a small error which distracts me from the main work, and which is very easy to fix. Fire-and-forget.

Vladimir Tsichevski wrote:

Greg Shah wrote:

I don't object to the change, but we don't want to spent time on making external files work at this time. We have many places like this which will not work with inputs that are outside of the project home. Please move everything inside the project.

It is a small error which distracts me from the main work, and which is very easy to fix. Fire-and-forget.

Fixed in 3821c rev. 14187.

Constantin Asofiei wrote:

Vladimir, please create a 4GL package for the class - I think FWD has problems converting .cls files without a package. So, change your TestClass as oo.TestClass, place it in a oo/ folder and convert again. Most likely this is the issue.

This did not help. Note: in testcases I converted the NoPackageClass.cls, which has no package.

Let's not spend time on conversion of OEUnit. We know we are not going to use it in FWD.

First, I see warnings like these:

This is a limitation in 3821c that is fixed in 6129a. 6129a currently causes regressions in multiple projects. Until those are resolved, the 6129a changes cannot be merged back to 3821c. Don't spend time on these warnings.

SourceNameMapper initMappingData

Does this happen after 3821c revision 14190?

Greg Shah wrote:

Let's not spend time on conversion of OEUnit. We know we are not going to use it in FWD.

First, I see warnings like these:

This is a limitation in 3821c that is fixed in 6129a. 6129a currently causes regressions in multiple projects. Until those are resolved, the 6129a changes cannot be merged back to 3821c. Don't spend time on these warnings.

OK

SourceNameMapper initMappingData

Does this happen after 3821c revision 14190?

Do not think so. The change seems unrelated.

I wonder what is the life cycle of the name_map.xml file.

A see this file is deleted in file system by ant target before conversion begins, after that it is loaded as a classpath resource in SourceNameMapper.initMappingData.

Who creates this file, and put it in classpath?

It is very unlikely that this is unrelated. It may just be an additional regression since SourceNameMapper is used at both conversion and runtime.

Who creates this file, and put it in classpath?

It is loaded from the application jar at runtime. It it not loaded at conversion time, it is a conversion output.

Tomasz: Please see #6237-30 for a possible regression from the recent SourceNameMapper changes.

Greg Shah wrote:

It is very unlikely that this is unrelated. It may just be an additional regression since SourceNameMapper is used at both conversion and runtime.

Who creates this file, and put it in classpath?

It is loaded from the application jar at runtime. It it not loaded at conversion time, it is a conversion output.

But I get this NPE while converting.

We create the name_map.xml during annotations. We used to store the data in a binary serialized object called name_map.cache but at some point we switched over to load the name_map.xml during the back-end processing.

The key thing here is that the recent changes are likely to have caused this problem.

If you test the version in revision 14184, I think the issue would be gone.

bzr cat -r14184 src/com/goldencode/p2j/util/SourceNameMapper.java > src/com/goldencode/p2j/util/SourceNameMapper.java

Greg Shah wrote:

We create the name_map.xml during annotations. We used to store the data in a binary serialized object called name_map.cache but at some point we switched over to load the name_map.xml during the back-end processing.

The key thing here is that the recent changes are likely to have caused this problem.

this code from initMappingData() causes NPE if called twice:

            // lookup the descriptors for the original file system
            pkgroot = Utils.getDirectoryNodeString(null, "legacy-system/pkgroot", "");

            // if the pkgroot is null get the custom package and reset it
            if (pkgroot == null || pkgroot.isEmpty())
            {
               pkgroot = custpkgroot;
               custpkgroot = null;
               ^^^^^^^^^^^^^^^^^^^ the code cannot be called again after that since null will be put to pkgroot
            }
            ...
            sb.append(pkgroot.replaceAll("\\.", StringHelper.fixupRegex(fileSep)));
                      ^^^^^^^ NPE

The overall logic is unclean (or smells?). This code is called from conversion rules. The name_map.xml does not exist at this point, so the method never succeed. Why is this? If this is OK for some reason, then that is the expected program reaction? Now the program calls this never succeeding method repeatedly, which is not good.

We took a short cut, which is why it smells. Yes, there is technical debt.

Can you use the version from rev 14184 without a problem? I really prefer you to work on implementing OEUnit/ABLUnit support.

Greg Shah wrote:

Tomasz: Please see #6237-30 for a possible regression from the recent SourceNameMapper changes.

It should be fixed in 3821c/14190, the change prevents recursive calls to initMappingData which combined with new locking makes sure initMappingData is executed only once for application lifecycle.
EDIT: It seems not fixed, working on it.

Greg Shah wrote:

We took a short cut, which is why it smells. Yes, there is technical debt.

Can you use the version from rev 14184 without a problem? I really prefer you to work on implementing OEUnit/ABLUnit support.

Seems so. Classes can be converted and compiled after I reverted the SourceNameMapper as you suggested.

OEUnit uses 4gl annotations to mark methods. I do not see these annotations in AST files. It seems, these annotation are ignored by FWD parser.
So, the first thing we should do is to fix this, correct?

Vladimir Tsichevski wrote:

Greg Shah wrote:

We took a short cut, which is why it smells. Yes, there is technical debt.

Can you use the version from rev 14184 without a problem? I really prefer you to work on implementing OEUnit/ABLUnit support.

Seems so. Classes can be converted and compiled after I reverted the SourceNameMapper as you suggested.

Vladimir, I've fixed the issue in rev 14194 and tested conversion in another project - it worked correctly. I hope it will work for your project as well.

Tomasz Domin wrote:

Vladimir Tsichevski wrote:

Greg Shah wrote:

We took a short cut, which is why it smells. Yes, there is technical debt.

Can you use the version from rev 14184 without a problem? I really prefer you to work on implementing OEUnit/ABLUnit support.

Seems so. Classes can be converted and compiled after I reverted the SourceNameMapper as you suggested.

Vladimir, I've fixed the issue in rev 14194 and tested conversion in another project - it worked correctly. I hope it will work for your project as well.

Yes, now I can convert again, thank you!

Vladimir Tsichevski wrote:

OEUnit uses 4gl annotations to mark methods. I do not see these annotations in AST files. It seems, these annotation are ignored by FWD parser.
So, the first thing we should do is to fix this, correct?

Hmmm. No, you should see an ANNOTATION node which has the annotation name as a first child (type SYMBOL), then optionally there is an LPARENS child which will have one or more ASSIGN nodes (one for each attribute).

Greg Shah wrote:

Vladimir Tsichevski wrote:

OEUnit uses 4gl annotations to mark methods. I do not see these annotations in AST files. It seems, these annotation are ignored by FWD parser.
So, the first thing we should do is to fix this, correct?

Hmmm. No, you should see an ANNOTATION node which has the annotation name as a first child (type SYMBOL), then optionally there is an LPARENS child which will have one or more ASSIGN nodes (one for each attribute).

The 4gl annotation nodes are inserted into AST at some conversion stage, then removed at some later stage :-(

Vladimir, the OE annotations (any kind) are processed in annotations/legacy_services.rules. At the time, I did not consider other kind of annotations, so they are dropped if they are not recognized for legacy web services.

• a small sample on how these annotations look in OE
• how is the converted code supposed to look, in Java, for these annotations?

Constantin Asofiei wrote:

Vladimir, the OE annotations (any kind) are processed in annotations/legacy_services.rules. At the time, I did not consider other kind of annotations, so they are dropped if they are not recognized for legacy web services.

I need some info from you:

• a small sample on how these annotations look in OE

Here it is:

ROUTINE-LEVEL ON ERROR UNDO, THROW.

CLASS oo.SimpleTest:

  @BeforeClass.
  METHOD PUBLIC VOID BeforeClass():
    // Do some stuff once before this class is loaded
  END METHOD. 

  @Before.
  METHOD PUBLIC VOID Before():
    // Do some stuff once before this class is executed
  END METHOD. 

  @Test.
  METHOD PUBLIC VOID Test():
    // Do some testing
  END METHOD. 

  @After.
  METHOD PUBLIC VOID After1():
    // Do some stuff once after this class is executed
  END METHOD.  

  @After.
  METHOD PUBLIC VOID After2():
    // Do some more stuff once after this class is executed
  END METHOD.  

  @AfterClass.
  METHOD PUBLIC VOID AfterClass1():
    // Do some stuff once before this class is loaded
  END METHOD.  

  @AfterClass.
  METHOD PUBLIC VOID AfterClass2():
    // Do some stuff once before this class is loaded
  END METHOD.  

END CLASS.

• how is the converted code supposed to look, in Java, for these annotations?

All annotated 4gl methods should be converted to annotated public void Java method with no arguments.

Below is the list of 4gl annotations and their JUnit 5 Java counterparts. If we would wanted to use JUnit 4, we would use the 4gl annotation names unchanged.

1. @Test -> @Test. Denotes that a method is a test method.

1. @Before -> @BeforeEach. Denotes that the annotated method should be executed before each @Test method in the current class.

1. @After -> @AfterEach. Denotes that the annotated method should be executed after each @Test

1. @BeforeClass -> @BeforeAll. Denotes that the annotated method should be executed before all @Test methods in the current class

1. @AfterClass -> @AfterAll. Denotes that the annotated method should be executed after all @Test methods in the current class

1. @Ignore -> @Disabled. Used to disable a test class or test method.

I wrote an additional rule-set which replaces OEUnit 4gl annotation names to corresponding JUnit 5 names.

The problem is in the annoNode.text = annoName instruction: it does not work, and the replacement does not occur (annotation reach Java unchanged).

<?xml version="1.0"?>
<rule-set>
   <worker class="com.goldencode.p2j.uast.ProgressPatternWorker" namespace="prog" />
   <ascent-rules>
      <rule>this.type == prog.annotation
            and parent.type == prog.method_def
         <variable name="annoName" type="java.lang.String" />
         <variable name="annoNode" type="com.goldencode.ast.Aast" />
         <action>annoNode = this.firstChild</action>

         <!-- Get initial annotation name -->
         <action>annoName = this.firstChild.text</action>

         <!-- Replace OEUnit (aka JUnit 4) annotation name to JUnit 5 name if needed -->
         <rule>annoName == "Before" 
            <action>annoName = "BeforeEach"</action>
            <rule on="false">annoName == "After" 
               <action>annoName = "AfterEach"</action>
               <rule on="false">annoName == "BeforeClass" 
                  <action>annoName = "BeforeAll"</action>
                  <rule on="false">annoName == "AfterClass" 
                     <action>annoName = "AfterAll"</action>
                     <rule on="false">annoName == "Ignore" 
                        <action>annoName = "Disabled"</action>
                     </rule>
                  </rule>
               </rule>
            </rule>
         </rule>

         <!-- Write back name to the annotation -->
         <action>annoNode.text = annoName</action>
      </rule>

   </ascent-rules>
</rule-set>

What am I doing wrong here? I know setting text works on the copy:

      /**
       * Set the token text of the current copy AST.
       *
       * @return  Always true.
       */
      public boolean setText(String text)
      {
         getCopy().setText(text);

         return true;
      }

Constantin, can you help me please?

Help needed.

I have to insert Java import statements. I gathered, I can use the ConverterHelper.createImport() for this purpose.
But if I do this at the "annotations.xml", no result is produced, since getResolver().getTargetRootAst() returns null, and the ConverterHelper.createImport() silently does nothing.

The question is: starting from which point in conversion flow can I add imports? What is the principal difference between adding Java annotations and adding Java import statements?

Another problem: if I move my conversion rules, which do JUnit annotations preparation, to the back conversion stage, I got conversion error with NPE for the same expression, which was converted OK at the "annotations.xml" stage. Why is this? Does this mean for the purpose I need two .rules files instead of one? The first will be called from "annotations.xml", and do Java annotations preparation, the second will be called from "conversion.xml" and will add Java import statements. Are the "annotations.xml" and "conversion.xml" the correct places to do this?

I think you are conflating the annotations phase of conversion (when annotations.xml and all its related rule-sets are run) with Java annotations. The annotations phase of conversion is about annotating the Progress ASTs created by earlier phases of conversion (like parsing and fixups) with information that is collected/calculated/derived about the Progress program represented by the Progress AST being walked. For instance, during this phase, we determine the block properties for blocks of 4GL code; we select which database index would have been selected by the 4GL runtime for a query; we compute the converted Java names which will be assigned to certain resources (variables, streams, buffers, etc.); and many more things...

All this information is collected and saved into AST annotations in the copy of the Progress AST being walked. You must not save AST annotations in the source AST. The source AST only drives the AST walk, but it is discarded at the end of the walk, so any information stored there will be lost. The copy AST is meant to be augmented with annotations; it becomes the source AST for the next phase of conversion, so that each phase can build off the previous one, with the benefit of having all the information discovered by the previous walk accessible as AST annotations.

No Java annotations or import statements are created at this point, because a parallel Java AST representing the converted program has not yet been created.

A parallel Java AST is created to represent the converted 4GL program during the business logic base structure phase. This Java AST is a "skeleton" AST which defines the structure of the Java class representing the converted business logic program. It has attach points for elements of the Java class, such as import statements, static and instance variable, constructors, methods, etc. These areas of the Java AST are filled in during the next phase of conversion.

During the core conversion phase, we flesh that skeleton out, based on the control flow of the original 4GL program (represented by the associated Progress AST), and the information gathered during the earlier annotations phase. It is during this phase that we do things like add import statements, variables, methods, Java annotations, etc. Once the Java AST is fully formed, we anti-parse it into Java source code in the last phase of conversion.

So yes, think of the annotations phase of conversion as the phase where things are "figured out" and stored for downstream use, and the conversion phase as the phase where the information figured out during annotations is used to create a fully formed Java AST.

W.r.t. adding Java import statements, the typical idiom we use during the conversion phase to add an optional import statement is to define a Boolean variable which is initialized to false. We then set it to true only if we match a walk rule which determines that the associated import statement is needed. This should be the same walk rule which adds the Java code which relies on the import statement. Then, during post-rules stage at the end of the Progress AST walk, we check the value of that boolean variable and if it is true, we use the createImport helper to add the import statement.

For example, see rules/convert/methods_attributes.rules. See the places the uiimport variable is used. It is initialized to false in <init-rules>, optionally set to true in <walk-rules>, and checked in <post-rules>. Only if it has been set to true is the import statement added.

I haven't added Java annotations to converted business logic, but this also would occur during the core conversion phase. Someone else will be able to point you to a good example of this.

Eric Faulhaber wrote:

I think you are conflating the annotations phase of conversion (when annotations.xml and all its related rule-sets are run) with Java annotations. The annotations phase of conversion is about annotating the Progress ASTs created by earlier phases of conversion (like parsing and fixups) with information that is collected/calculated/derived about the Progress program represented by the Progress AST being walked. For instance, during this phase, we determine the block properties for blocks of 4GL code; we select which database index would have been selected by the 4GL runtime for a query; we compute the converted Java names which will be assigned to certain resources (variables, streams, buffers, etc.); and many more things...

All this information is collected and saved into AST annotations in the copy of the Progress AST being walked. You must not save AST annotations in the source AST. The source AST only drives the AST walk, but it is discarded at the end of the walk, so any information stored there will be lost. The copy AST is meant to be augmented with annotations; it becomes the source AST for the next phase of conversion, so that each phase can build off the previous one, with the benefit of having all the information discovered by the previous walk accessible as AST annotations.

No Java annotations or import statements are created at this point, because a parallel Java AST representing the converted program has not yet been created.

A parallel Java AST is created to represent the converted 4GL program during the business logic base structure phase. This Java AST is a "skeleton" AST which defines the structure of the Java class representing the converted business logic program. It has attach points for elements of the Java class, such as import statements, static and instance variable, constructors, methods, etc. These areas of the Java AST are filled in during the next phase of conversion.

During the core conversion phase, we flesh that skeleton out, based on the control flow of the original 4GL program (represented by the associated Progress AST), and the information gathered during the earlier annotations phase. It is during this phase that we do things like add import statements, variables, methods, Java annotations, etc. Once the Java AST is fully formed, we anti-parse it into Java source code in the last phase of conversion.

So yes, think of the annotations phase of conversion as the phase where things are "figured out" and stored for downstream use, and the conversion phase as the phase where the information figured out during annotations is used to create a fully formed Java AST.

W.r.t. adding Java import statements, the typical idiom we use during the conversion phase to add an optional import statement is to define a Boolean variable which is initialized to false. We then set it to true only if we match a walk rule which determines that the associated import statement is needed. This should be the same walk rule which adds the Java code which relies on the import statement. Then, during post-rules stage at the end of the Progress AST walk, we check the value of that boolean variable and if it is true, we use the createImport helper to add the import statement.

For example, see rules/convert/methods_attributes.rules. See the places the uiimport variable is used. It is initialized to false in <init-rules>, optionally set to true in <walk-rules>, and checked in <post-rules>. Only if it has been set to true is the import statement added.

I haven't added Java annotations to converted business logic, but this also would occur during the core conversion phase. Someone else will be able to point you to a good example of this.

Thanks, Eric, for this excellent explanation. I am sure this material must be added to the TRPL docs. I have not read anything like this in the TRPL docs.

What am I doing wrong here? I know setting text works on the copy:

The problem is subtle. The setText() you are referencing is a kind of global variable that can be referenced as text = <expr>. You are not using this. You are using annoNode.text = annoName which is the Aast.setText() method call on the annoNode which is NOT the copy AST, but the source AST. So you are modifying the source AST and those changes are lost.

If you change your code from <action>annoNode = this.firstChild</action> to <action>annoNode = copy.firstChild</action>, then it would work.

I am sure this material must be added to the TRPL docs. I have not read anything like this in the TRPL docs.

Much of this is specific to FWD, not to TRPL. We would put these in the Conversion Reference or in Internals.

The first will be called from "annotations.xml", and do Java annotations preparation, the second will be called from "conversion.xml" and will add Java import statements.

Yes

Are the "annotations.xml" and "conversion.xml" the correct places to do this?

Yes

Greg Shah wrote:

What am I doing wrong here? I know setting text works on the copy:

The problem is subtle. The setText() you are referencing is a kind of global variable that can be referenced as text = <expr>. You are not using this. You are using annoNode.text = annoName which is the Aast.setText() method call on the annoNode which is NOT the copy AST, but the source AST. So you are modifying the source AST and those changes are lost.

Got it.

If you change your code from <action>annoNode = this.firstChild</action> to <action>annoNode = copy.firstChild</action>, then it would work.

No, this does not work, but for another reason: for the Java annotation name, the annotation node AST annotatation javaName is used, not the annotation symbol text.

So, the working code should look like this:

   <walk-rules>
      <rule>this.type == prog.annotation
            and parent.type == prog.method_def
         <action>copy.putAnnotation("javaname", "SomJavaAnnotationName")</action>
      </rule>
     ...

The question is in which module this javaName is assigned? It would be better to calculate the correct value for this annotation there instead of patching it afterwords.

Now that I have all Junit5 annotations in place, I have another problem when I try to run the SimpleTest class as JUnit test: EmptyStackException:

org.eclipse.jdt.internal.junit.runner.RemoteTestRunner at localhost:33795    
    Thread [main] (Suspended (exception EmptyStackException))    
        Stack<T>.peek() line: 119    
        TransactionManager.deregisterOutputParameterAssigner(TransactionManager$WorkArea) line: 7018    
        TransactionManager.access$53(TransactionManager$WorkArea) line: 7015    
        TransactionManager$TransactionHelper.deregisterOutputParameterAssigner() line: 9780    
        ObjectOps.loadClass(Class<_BaseObject_>) line: 1976    
        ObjectOps.load(Class<_BaseObject_>) line: 1492    
        BlockManager.checkJavaCall(WorkArea, Class<?>, Object, String, Block, boolean, boolean, boolean) line: 8472    
        BlockManager.topLevelBlock(Class<?>, Object, String, TransactionType, BlockType, Block) line: 8215    
        BlockManager.internalProcedure(Object, String, BlockManager$TransactionType, Block) line: 695    
        BlockManager.internalProcedure(Object, String, Block) line: 672    
        SimpleTest.beforeClass() line: 40    
        NativeMethodAccessorImpl.invoke0(Method, Object, Object[]) line: not available [native method]    
        NativeMethodAccessorImpl.invoke(Object, Object[]) line: 62    
        DelegatingMethodAccessorImpl.invoke(Object, Object[]) line: 43    
        Method.invoke(Object, Object...) line: 498    
        ReflectionUtils.invokeMethod(Method, Object, Object...) line: 725    
        MethodInvocation<T>.proceed() line: 60    
        InvocationInterceptorChain$ValidatingInvocation<T>.proceed() line: 131    
        TimeoutExtension.intercept(Invocation<T>, ReflectiveInvocationContext<Method>, ExtensionContext, TimeoutDuration, TimeoutProvider) line: 149    
        TimeoutExtension.interceptLifecycleMethod(Invocation<Void>, ReflectiveInvocationContext<Method>, ExtensionContext, TimeoutProvider) line: 126    
        TimeoutExtension.interceptBeforeAllMethod(Invocation<Void>, ReflectiveInvocationContext<Method>, ExtensionContext) line: 68    
        580718781.apply(InvocationInterceptor, InvocationInterceptor$Invocation, ReflectiveInvocationContext, ExtensionContext) line: not available    
        ExecutableInvoker$ReflectiveInterceptorCall<E,T>.lambda$ofVoidMethod$0(ExecutableInvoker$ReflectiveInterceptorCall$VoidMethodInterceptorCall, InvocationInterceptor, InvocationInterceptor$Invocation, ReflectiveInvocationContext, ExtensionContext) line: 115    
        149047107.apply(InvocationInterceptor, InvocationInterceptor$Invocation, ReflectiveInvocationContext, ExtensionContext) line: not available    
        ExecutableInvoker.lambda$invoke$0(ExecutableInvoker$ReflectiveInterceptorCall, ReflectiveInvocationContext, ExtensionContext, InvocationInterceptor, InvocationInterceptor$Invocation) line: 105    
        71587369.apply(InvocationInterceptor, InvocationInterceptor$Invocation) line: not available    
        InvocationInterceptorChain$InterceptedInvocation<T>.proceed() line: 106    
        InvocationInterceptorChain.proceed(Invocation<T>) line: 64    
        InvocationInterceptorChain.chainAndInvoke(Invocation<T>, InterceptorCall<T>, List<InvocationInterceptor>) line: 45    
        InvocationInterceptorChain.invoke(Invocation<T>, ExtensionRegistry, InterceptorCall<T>) line: 37    
        ExecutableInvoker.invoke(Invocation<T>, ReflectiveInvocationContext<E>, ExtensionContext, ExtensionRegistry, ReflectiveInterceptorCall<E,T>) line: 104    
        ExecutableInvoker.invoke(Method, Object, ExtensionContext, ExtensionRegistry, ReflectiveInterceptorCall<Method,T>) line: 98    
        ClassTestDescriptor(ClassBasedTestDescriptor).lambda$invokeBeforeAllMethods$11(Method, Object, ExtensionContext, ExtensionRegistry) line: 397    
        769429195.execute() line: not available    
        OpenTest4JAndJUnit4AwareThrowableCollector(ThrowableCollector).execute(ThrowableCollector$Executable) line: 73    
        ClassTestDescriptor(ClassBasedTestDescriptor).invokeBeforeAllMethods(JupiterEngineExecutionContext) line: 395    
        ClassTestDescriptor(ClassBasedTestDescriptor).before(JupiterEngineExecutionContext) line: 209    
        ClassTestDescriptor(ClassBasedTestDescriptor).before(EngineExecutionContext) line: 80    
                ...

Note: this stacktrace is produced when I run the test from Eclipse, but the problem is not related to Eclipse: same exception is thrown if I run this as standalone JUnit console test runner.

This problem arises both for instance methods (for example, @Test methods) and class methods (for example, @BeforeAll

I suppose, the Java method, produced by conversion of a 4gl class, must be called in some special way?

The question is in which module this javaName is assigned? It would be better to calculate the correct value for this annotation there instead of patching it afterwords.

Do you still need an answer for this?

Vladimir Tsichevski wrote:

I suppose, the Java method, produced by conversion of a 4gl class, must be called in some special way?

Yes. FWD is a kind of 4GL-compatible container which runs as its own application server, right? We can't just execute the converted Java code. It must run inside that container and must be executed with a very specific context which is normally setup as a FWD client which calls StandardServer.standardEntry().

As noted in #6237-3, we plan to implement the design from #3827-6 in which we implement our own TestEngine. The JUnit TestEngine needs to be able to invoke code like a FWD client. We could spawn a FWD client and build an IPC connection to submit jobs and get responses. However, I think that is overcomplicated and I don't want to go that way.

I think the way to go is to create a FWD client in-process from the JUnit TestEngine code:

1. Rework the ClientCore.start().
   • The parameter list should take a lambda which implements the core logic to be executed for the session.
   • The current implementation would just move running = main.standardEntry(); to that lambda. We can create a helper that implements the current method signature and which passes this code as the lambda. That way all existing code runs unchanged.
   • The new "worker" method that takes the lambda would then be used by the TestEngine code.
2. Implement a server-side RemoteObject which implements any needed services such as scanning for converted code that implements given annotations.
3. The TestEngine code would:
   • Gather the necessary configuration (a BootstrapConfig instance) to pass to the ClientCore.start(). We would force batch mode for the session (and any other critical values) but some configuration will be needed to make the proper connection to the server. This will be a runtime thing. We need to discuss how this is encoded/passed to the TestEngine. I want to reuse our existing approaches to configuration (e.g. using the boostrap config XML file) unless there is a really good reason to do something else.
   • Define the lambda as a executor that processes jobs posted to a queue.
   • Spawn a thread and call ClientCore.start() to initialize the FWD client and start the executor.
   • Post service requests to the queue to implement the features needed in the TestEngine. This would call the API provided by the server-side RemoteObject to get things implemented.

Does this make sense?

Please rename both oeunit.rules files to unit_test_support.rules. Remember that we are implementing support for both ABLUnit and OEUnit. I expect the support to be common code with minor specializations for the differences.

Vladimir: Please post questions or comments here. As a next step, I'd like to see the proposed design of the RemoteObject API needed to implement the TestEngine.

In addition, I think there is a key question about how OEUnit and ABLUnit work: do they run inside a single 4GL long-lived process or do they spawn a 4GL client for each test? The approach I've suggested is the long-lived single process approach (which seems most natural), but if it is a poor match then we may need some tweaks to the proposed design.

Greg Shah wrote:

Please rename both oeunit.rules files to unit_test_support.rules. Remember that we are implementing support for both ABLUnit and OEUnit. I expect the support to be common code with minor specializations for the differences.

Done. If you have no objections, I will commit this change.

Greg Shah wrote:

The question is in which module this javaName is assigned? It would be better to calculate the correct value for this annotation there instead of patching it afterwords.

Do you still need an answer for this?

If I get the answer, I could probably move the code to a better place, so other could locate it easier in the future. Besides, it is always better to create things correctly than create it incorrectly in one piece of the code first and fix them later in another piece.

Greg Shah wrote:

Does this make sense?

If I get it right, then:

1. We can execute on the server side anything resulted from 4gl conversion only in a client thread, so we need a client
2. The JUnit5 testing will happen in a special client process. This process will have JUnit5 jars in its classpath.
3. We need to create a JUnit5 extension for our purposes
4. This extension will do everything remotely on the server

To implement this:

1. We will need a specialized RemoteObject API
2. Since every activity is usually initiated from the server side, we will need to use something which will allow us to do reverse thing: initiate calls from the JUni5 engine at the client to the server.

In TRPL, are we mapping annoName == "Before" to String.equalsIgnoreCase()? If not then shouldn't we do that explicitly here?

Otherwise I'm OK with these additions being checked in.

Vladimir Tsichevski wrote:

Greg Shah wrote:

Does this make sense?

If I get it right, then:

1. We can execute on the server side anything resulted from 4gl conversion only in a client thread, so we need a client
2. The JUnit5 testing will happen in a special client process. This process will have JUnit5 jars in its classpath.
3. We need to create a JUnit5 extension for our purposes
4. This extension will do everything remotely on the server

To implement this:

1. We will need a specialized RemoteObject API
2. Since every activity is usually initiated from the server side, we will need to use something which will allow us to do reverse thing: initiate calls from the JUni5 engine at the client to the server.

Yes, I think you have the correct understanding.

Greg Shah wrote:

In TRPL, are we mapping annoName == "Before" to String.equalsIgnoreCase()?

I did not know this and cannot find this in the docs :( What is the correct TRPL way to compare strings case-sensitive? Use annoName.equals("Before") ?

I always use the String methods directly (equals() or equalsIgnoreCase()). Even if we did implement string content comparison in == (instead of reference comparison), we would not be doing it case-insensitively.

• please format annotations/unit_test_support.rules by indenting the nested rules tags - it's hard to read the code without proper formatting
• regarding the regression in #5034-2722 - any other annotations which is not part of legacy_services or unit_test_support must be dropped. Do this by marking the annotation AST with a fwd="true", for all existing annotations in the 4GL code which were prepared to be emitted in the converted code, or any created prog.annotation AST - look for code like createProgressAst(prog.annotation, ...) in legacy_services and others.
• add another rule-set named like legacy_annotations_post.rules in annotations.xml, after the unit_test_support and legacy_services, which just drops any prog.annotation node which does not have the fwd="true" annotation.

Constantin Asofiei wrote:

Regarding 3821c/14239:

• please format annotations/unit_test_support.rules by indenting the nested rules tags - it's hard to read the code without proper formatting

This formatting was intentional: the construction is really a switch implemented as a series if if... elseif statements.
Reverted to the original formatting.

Do this by marking the annotation AST with a fwd="true", for all existing annotations in the 4GL code which were prepared to be emitted in the converted code, or any created prog.annotation AST - look for code like createProgressAst(prog.annotation, ...) in legacy_services and others.

• add another rule-set named like legacy_annotations_post.rules in annotations.xml, after the unit_test_support and legacy_services, which just drops any prog.annotation node which does not have the fwd="true" annotation.

Done in rev. 14241.

legacy_services.rules is built to allow LegacyService annotations written directly in FWD code - these must be kept. See the 'keep' variable:

            <rule>lower(lname) == "legacyservice" 
               <action>keep = true</action>
            </rule>

Constantin Asofiei wrote:

legacy_services.rules is built to allow LegacyService annotations written directly in FWD code - these must be kept. See the 'keep' variable:
[...]
I think it should be enough to have ref.putAnnotation("fwd", true) if keep = true.

Are you saying the code in legacy_services.rules is broken now? Or just suggest optimization?

Vladimir Tsichevski wrote:

Constantin Asofiei wrote:

legacy_services.rules is built to allow LegacyService annotations written directly in FWD code - these must be kept. See the 'keep' variable:
[...]
I think it should be enough to have ref.putAnnotation("fwd", true) if keep = true.

Are you saying the code in legacy_services.rules is broken now? Or just suggest optimization?

The code previously meant to allow @LegacyService annotation in the 4GL code. Now it does not. I don't know if any customer uses this, but please fix it. Just use this LegacyService name at the 4GL code.

Constantin Asofiei wrote:

The code previously meant to allow @LegacyService annotation in the 4GL code. Now it does not.

Really it does. I've added the @LegacyService annotation to SimpleTest.cls, and it was successfully propagated to java.

I don't know if any customer uses this, but please fix it. Just use this LegacyService name at the 4GL code.

I have not found any usages of @LegacyService in customer code I know.

To add JUnit support to converted projects, the following patch has to be applied to build.gradle:

=== modified file 'build.gradle'
--- build.gradle    2022-08-17 06:40:50 +0000
+++ build.gradle    2022-09-14 15:58:09 +0000
@@ -580,7 +580,10 @@
        aspectjCompile group: 'org.aspectj', name: 'aspectjweaver', version: aspectjVersion
     }

-    fwdAllTest group: 'org.junit.platform', name: 'junit-platform-console-standalone', version: '1.7.1'
+    // Adding JUnit5 jar both to client and server, so it will be deployed to converted projects too
+    fwdClientServer group: 'org.junit.platform',
+                    name: 'junit-platform-console-standalone',
+                    version: '1.9.0'
 }

 // inject ant-optional dependencies to ant, this is needed for the antlr ant target

Previously the JUnit5 jar was used for tests only, now it is installed along with other FWD libraries, so it will be copied to converted projects too.

The latest problem reveals that we need to convert 4gl procedure annotation either.

So we need to answer the following questions:

1. Whether or not and how class-level annotations (@BeforeClass, @AfterClass) are used in 4gl procedures?
2. If a 4gl class method is used as a test method, a new class instance should be usually created for each test? What about 4gl procedures?

1. Whether or not and how class-level annotations (@BeforeClass, @AfterClass) are used in 4gl procedures?

Assume that all features of OEUnit are in use. As to how they are used, you should write testcases.

1. If a 4gl class method is used as a test method, a new class instance should be usually created for each test? What about 4gl procedures?

You'll have to write tests to see.

Vladimir Tsichevski wrote:

So we need to answer the following questions:

1. Whether or not and how class-level annotations (@BeforeClass, @AfterClass) are used in 4gl procedures?

OEUnit does not support test procedures, it must be a class... this is the only OEUnit I'm aware of - [[https://github.com/CameronWills/OEUnit]]

1. If a 4gl class method is used as a test method, a new class instance should be usually created for each test? What about 4gl procedures?

Yes, the Runner takes care of test class/suite instantiation and one instance is created for each test run. Procedures are not supported, unless you're talking about a different OEUnit.

Marian Edu wrote:

this is the only OEUnit I'm aware of - [[https://github.com/CameronWills/OEUnit]]

Yes, I mean this very OEUnit project.

We have some difficulty if we are going to implement both OEUnit and ABLUnit.

There is a difference in the usage of @Before and @After annotations:

1. in OEUnit the @Before and @After annotations mean before and after a test
2. in ABLUnit the @Before and @After annotations mean before and after a test class.

So, when converting these annotations to Java annotations, we should know for which of two frameworks the 4gl source were written.
Alternatively, we may test if we are dealing with a STATIC method, but this seems fragile to me.

Define a global parameter (in cfg/p2j.cfg.xml) for OEUnit vs ABLUnit. Then add an override in the UastHints for the same value. That will allow the default to be set on a project-wide basis, but when there is a project that has both in use then we can handle it. Although our existing customer projects only use one or the other, we know that our own testcases project is going to have tests for both.

Greg Shah wrote:

Define a global parameter (in cfg/p2j.cfg.xml) for OEUnit vs ABLUnit. Then add an override in the UastHints for the same value. That will allow the default to be set on a project-wide basis, but when there is a project that has both in use then we can handle it. Although our existing customer projects only use one or the other, we know that our own testcases project is going to have tests for both.

How global parameters in cfg/p2j.cfg.xml do relate to uast hints? If I add a parameter to uast hints, do I need to update UastHints.java and the class Javadoc for the new parameter?

Will I need to read both cfg/p2j.cfg.xml parameters and uast hints in TRPL code as two different operations, or there is one combined read operation?

How global parameters in cfg/p2j.cfg.xml do relate to uast hints?

They are read from UastHints as the default value. See how UastHints.marker and UastHints.defaultMarker() are implemented. Then notice how marker can be set from UastHints.readPreproc() which reads the XML hints configuration. Your value wouldn't live in the preprocessor element but the concept is the same.

If I add a parameter to uast hints, do I need to update UastHints.java and the class Javadoc for the new parameter?

Yes.

Will I need to read both cfg/p2j.cfg.xml parameters and uast hints in TRPL code as two different operations, or there is one combined read operation?

No, it can be a single call but you will have to expose it as a method in UastHintsWorker.HintsReader.

I am implementing a specialized JUnit5 test engine for FWD. The code works, and now I need some permanent Java package name to place the code in. It can be com.goldencode.testengine or com.goldencode.p2j.testengine.

Greg, what would you propose?

The code is going to be very specific to converted 4GL code so it should be com.goldencode.p2j.testengine.

Greg Shah wrote:

The code is going to be very specific to converted 4GL code so it should be com.goldencode.p2j.testengine.

OK

Also, we need to change the FWD test code in the test/ sub-directory to pure JUnit5. Now they are written for JUnit4 and are run by JUnit5 Jupiter engine in backward compatibility mode.

Greg Shah wrote:

Define a global parameter (in cfg/p2j.cfg.xml) for OEUnit vs ABLUnit. Then add an override in the UastHints for the same value. That will allow the default to be set on a project-wide basis, but when there is a project that has both in use then we can handle it. Although our existing customer projects only use one or the other, we know that our own testcases project is going to have tests for both.

Wouldn't checking package name for the Assert class used in tests already help in differentiating between the testing framework being used? I might be wrong here but I haven't seen any test class without at least one assertion. Although possible to always use full name on each reference I would say is fairly safe to assume there is one using statement, or maybe check if any class from the OEUnit package is found in the source code - this is not provided by PSC so it should be in the PROPATH during conversion.

Marian Edu wrote:

Greg Shah wrote:

Define a global parameter (in cfg/p2j.cfg.xml) for OEUnit vs ABLUnit. Then add an override in the UastHints for the same value. That will allow the default to be set on a project-wide basis, but when there is a project that has both in use then we can handle it. Although our existing customer projects only use one or the other, we know that our own testcases project is going to have tests for both.

Wouldn't checking package name for the Assert class used in tests already help in differentiating between the testing framework being used? I might be wrong here but I haven't seen any test class without at least one assertion. Although possible to always use full name on each reference I would say is fairly safe to assume there is one using statement, or maybe check if any class from the OEUnit package is found in the source code - this is not provided by PSC so it should be in the PROPATH during conversion.

I think, our goal is to have neither original OEUnit nor ABLUnit in the PROPATH at conversion time, because we do not want to use them in our future conversion environment. So we better use heuristics such as the annotation set we see in the converted sources, which tell us which kind of source we are converting (if we spot the Setup or TearDown, that means ABLUnit, if we see BeforeClass or AfterClass, that means OEUnit).

Or, as I proposed earlier, we just test if the annotated method is STATIC. This also will say us the meaning of the annotation without even bothering about which unit-testing framework this annotation belonged to.

Vladimir Tsichevski wrote:

I think, our goal is to have neither original OEUnit nor ABLUnit in the PROPATH at conversion time, because we do not want to use them in our future conversion environment.

Vladimir, I'm not saying the conversion needs those sources to be there but to look into the test class source code and see if there is any USING statement for any of the OEUnit class... if there isn't then you can assume ABLUnit.

Or, as I proposed earlier, we just test if the annotated method is STATIC. This also will say us the meaning of the annotation without even bothering about which unit-testing framework this annotation belonged to.

Not sure about this, in OEUnit the test/before/after methods can be static, it's not a requirement and in fact most of the time static is not used.

Marian Edu wrote:

Vladimir Tsichevski wrote:

I think, our goal is to have neither original OEUnit nor ABLUnit in the PROPATH at conversion time, because we do not want to use them in our future conversion environment.

Vladimir, I'm not saying the conversion needs those sources to be there but to look into the test class source code and see if there is any USING statement for any of the OEUnit class... if there isn't then you can assume ABLUnit.

Or, as I proposed earlier, we just test if the annotated method is STATIC. This also will say us the meaning of the annotation without even bothering about which unit-testing framework this annotation belonged to.

Not sure about this, in OEUnit the test/before/after methods can be static, it's not a requirement and in fact most of the time static is not used.

So, since neither of these methods is reliable, I propose using a configuration parameter then, as discussed above. If we have met any unit testing annotation, but no unit testing framework is explicitly configured, then we either:

1. throw an error and abort conversion
2. emit a warning and ignore the unit test annotations.

emit a warning and ignore the unit test annotations.

This one.

Vladimir Tsichevski wrote:

So, since neither of these methods is reliable

I really fail to see the issue here, isn't not just the annotations that needs to be converted but also the test methods and in those test methods there must be some kind of assertion if it's really a test. Based on the actual Assert class used there it should be obvious which testing framework is used in the 4GL code being converted.

Now, since the 'migration document' was already referenced here before I would only like to bring to your attention there are differences in the Assert API used. It's not clear if you plan to use the Assert class that was already made available in FWD and catch AssertionFailedError or just convert the asserts to direct JUnit ones, since the test methods can contain any 4GL code imho those would need to be translated as-is. And if this is the approach then adding implementation for OEUnit Assert in FWD would be the easier path.

adding implementation for OEUnit Assert in FWD would be the easier path.

We will provide the OEUnit Assert support.

Marian Edu wrote:

Vladimir Tsichevski wrote:

So, since neither of these methods is reliable

I really fail to see the issue here, isn't not just the annotations that needs to be converted but also the test methods and in those test methods there must be some kind of assertion if it's really a test.

So, you propose to pre-scan all classes in conversion project to find any assertXXX statement before doing anything else with the classes, then mark all ASTs containing tests?

And this will not be 100%-reliable still, since in theory, there might be valid test projects with no such indicator methods, or, there might be projects containing both kinds of unit-testing packages.

Also, this approach makes incremental conversion impossible.

you propose to pre-scan all classes in conversion project to find any assertXXX statement

This is pretty easy. We just add a rule-set to the annotations pipeline before unit_test_support.rules. All we have to do is look for access to any class in the OEUnit or ABLUnit "namespaces". This is not hard.

before doing anything else with the classes,

This part is handled by being in the pipeline early.

then mark all ASTs containing tests?

We just need to add an annotation at the root node. This can be read at the init-rules for unit_test_support.rules and you are good to go.

The key thing here is that it should be marked on a per-file basis. The assumption is that you cannot use both projects in a single program.

And this will not be 100%-reliable still, since in theory, there might be valid test projects with no such indicator methods,

This is the biggest issue. It is why we may still need a configuration flag.

or, there might be projects containing both kinds of unit-testing packages.

This is not an issue. As noted above, the annotation is stored per-file so different files can have different values.

Also, this approach makes incremental conversion impossible.

No. I don't think there is a limitation here.

Vladimir Tsichevski wrote:

So, you propose to pre-scan all classes in conversion project to find any assertXXX statement before doing anything else with the classes, then mark all ASTs containing tests?

Why all classes, as far as I know every class referenced in a source is going to be loaded anyway and by the time you want to inject annotations I expect that information to be already available in the AST, if any of those classes is from the OEUnit package then I would say you can treat it as a OEUnit test class.

And this will not be 100%-reliable still, since in theory, there might be valid test projects with no such indicator methods, or, there might be projects containing both kinds of unit-testing packages.

Again, I'm not saying you should look at the whole project and decide for each file individually. A test project with no assertion isn't really a test, so in theory that might be possible but then why even bother... just consider one (ABLUnit) the default and if you really find tests with annotations but no assertions assume the default, it will really make no difference :)

Also, this approach makes incremental conversion impossible.

That I can't comment on.

Marian Edu wrote:

Vladimir Tsichevski wrote:

So, you propose to pre-scan all classes in conversion project to find any assertXXX statement before doing anything else with the classes, then mark all ASTs containing tests?

Why all classes, as far as I know every class referenced in a source is going to be loaded anyway and by the time you want to inject annotations I expect that information to be already available in the AST, if any of those classes is from the OEUnit package then I would say you can treat it as a OEUnit test class.

Because the possibility of no assertion classes is referenced in a single test class is much larger than that for an entire project. So the possibility of a wrong decision.

I think we need to know on a per-file level which unit testing framework is used. This is not hard to do. As noted above, in most cases it can be calculated at annotations time from the code. Any cases that cannot be calculated can rely upon a configuration flag.

Greg Shah wrote:

I think we need to know on a per-file level which unit testing framework is used. This is not hard to do. As noted above, in most cases it can be calculated at annotations time from the code. Any cases that cannot be calculated can rely upon a configuration flag.

If we do rely upon a configuration parameter defined at per-project level only, and assuming that a single project can have both types of unit testing frameworks, we still do not solve the "heuristics does not work if a class has no assert methods used" problem.

If we can define this configuration parameter at any level, using also file and directory hints, then we do not need any (complicated and unreliable) heuristic method whatsoever.

If we can define this configuration parameter at any level, using also file and directory hints, then we do not need any (complicated and unreliable) heuristic method whatsoever.

True. But if we can calculate the value nearly 100% of the time, that will reduce the amount of configuration required to a very small amount.

On the other hand, directory-level hints will likely be enough to handle this requirement. For now, just implement the multilevel configuration approach. If it gets to the point where we are better off calculating things, then we can add that later.

Greg Shah wrote:

For now, just implement the multilevel configuration approach. If it gets to the point where we are better off calculating things, then we can add that later.

Completely agreed with this approach.

Vladimir Tsichevski wrote:

Greg Shah wrote:

For now, just implement the multilevel configuration approach. If it gets to the point where we are better off calculating things, then we can add that later.

Completely agreed with this approach.

I patched uast hints for this. The patch attached as uast-hints-unit-test-type.diff.

There is no patch posted for the hints changes.

Greg Shah wrote:

I think the way to go is to create a FWD client in-process from the JUnit TestEngine code:

1. Rework the ClientCore.start().
   • The parameter list should take a lambda which implements the core logic to be executed for the session.
   • The current implementation would just move running = main.standardEntry(); to that lambda. We can create a helper that implements the current method signature and which passes this code as the lambda. That way all existing code runs unchanged.

Does that mean we will not use the StandardServer at all?

• The new "worker" method that takes the lambda would then be used by the TestEngine code.

1. Implement a server-side RemoteObject which implements any needed services such as scanning for converted code that implements given annotations.
2. The TestEngine code would:
   • Gather the necessary configuration (a BootstrapConfig instance) to pass to the ClientCore.start(). We would force batch mode for the session

What does "batch mode" mean?

• Define the lambda as a executor that processes jobs posted to a queue.
• Spawn a thread and call ClientCore.start() to initialize the FWD client and start the executor.

Why do we need another thread?

Does that mean we will not use the StandardServer at all?

Maybe. At least StandardServer.standardEntry() is not designed for unit testing and I don't think it is a good fit for adding unit testing.

What does "batch mode" mean?

Some clients are not interactive. These are known as batch mode. This is a concept in the 4GL. You can run a batch mode client explicitly or you can run something like an appserver/REST/SOAP server-side agent which is implicitly batch mode.

I've been assuming that the JUnit stuff is implicitly batch mode, but perhaps it is possible to run interactive unit tests in OEUnit/ABLUnit? If so then we will have to plan for what that means.

Why do we need another thread?

It seems incorrect to "take over" the calling thread to process jobs (a.k.a. tests) submitted to the queue.

Greg Shah wrote:

Why do we need another thread?

It seems incorrect to "take over" the calling thread to process jobs (a.k.a. tests) submitted to the queue.

Do we need another thread on the client side only?

Greg Shah wrote:

I've been assuming that the JUnit stuff is implicitly batch mode, but perhaps it is possible to run interactive unit tests in OEUnit/ABLUnit?

There is not restriction on having interactive code (aka UI) in unit tests in ABL - any wait-for/prompt-for will block the test execution. However, if the user input is being used in tests I don't really see how relevant those tests are :)

Normally no UI code is tested, this doesn't mean the code must run in batch-mode, those investing time in writing unit tests don't just run them from development environment but from CI/CD pipelines using PCT tasks so really no interactive mode is supported (albeit, as said, not explicitly forbidden).

Vladimir Tsichevski wrote:

Greg Shah wrote:

Why do we need another thread?

It seems incorrect to "take over" the calling thread to process jobs (a.k.a. tests) submitted to the queue.

Do we need another thread on the client side only?

Yes. The new client session will have its normal conversation thread on the server that will handle any test execution. It would be blocked if there was no test executing.

Greg Shah wrote:

Vladimir Tsichevski wrote:

Greg Shah wrote:

Why do we need another thread?

It seems incorrect to "take over" the calling thread to process jobs (a.k.a. tests) submitted to the queue.

Do we need another thread on the client side only?

Yes. The new client session will have its normal conversation thread on the server that will handle any test execution. It would be blocked if there was no test executing.

So, the flow is:

1. We start the special unit test remote protocol
2. Using this remote protocol, we run test discovering
3. We start "normal conversation thread"
4. Using the remote protocol, we run tests.
5. we stop the client

Not exactly. The conversation thread starts when the session is created with the server. Everything else (steps 1, 2, 4 and 5) happen after the session is started.

Question: currently a remote instance of MainEntry is not only obtained inside the ClientCore.start(), but some manipulations are executed with it:

   public static void start(BootstrapConfig config, boolean file, boolean single, Supplier<String> diag)
   throws Exception
   {
      ...
      while (running)
      {
         MainEntry       main    = null;
         ...
         if (single)
         {
            // client code is "right" side by convention
            main = (MainEntry) RemoteObject.obtainLocalInstance(MainEntry.class, false);
         }
         else
         {
            main = (MainEntry) RemoteObject.obtainNetworkInstance(MainEntry.class, session);
         }

         String projectToken = config.getString("client", "project", "token", null);
         // this will set the token from either client:project:token or directory's project_token.
         main.setProjectToken(projectToken);

         String iniFileName = ...
         ThinClient.initializePost(config, session, context, main.loadStartupParameters(), iniFileName);

         ClientParameters params = ...
         main.setClientParams(params);
         ...
         ThinClient.getInstance().setUserName(params.osUserName);
         running = main.standardEntry();
         ...

If we just replace the call main.standardEntry() by some lambda passed to the method as a parameter, we at least lose access to all this initialization done with the instance.

May be, it will be better to create several variants of MainEntry and some hierarchy of classes and interfaces around it?

If we just replace the call main.standardEntry() by some lambda passed to the method as a parameter, we at least lose access to all this initialization done with the instance.

I'm not exactly sure what you are referring to.

I think the calls to main.setProjectToken(projectToken);, main.loadStartupParameters() and main.setClientParams(params); should remain as they are. There is no reason to change those.

In the case of main.standardEntry(), that server-side code is of limited value for unit testing. It is possible that some of it might be needed, but most of it will not be used. For example, the lookup processing for the entry point is not needed for unit testing.

May be, it will be better to create several variants of MainEntry and some hierarchy of classes and interfaces around it?

I'm not sure. We will need a specific idea of the code from StandardServer.standardEntry() which needs to be shared. Then we can discuss the approach.

Greg Shah wrote:

If we just replace the call main.standardEntry() by some lambda passed to the method as a parameter, we at least lose access to all this initialization done with the instance.

I'm not exactly sure what you are referring to.

I mean, if we replace the execution of a method of main by execution of an arbitrary lambda function, the access to the contents of the main object will be lost: we will not be able to access it from the lambda. At the moment I do not understand the code yet, so I do not know if such contents does exit, so I cannot say if this will be a problem or not.

In the case of main.standardEntry(), that server-side code is of limited value for unit testing. It is possible that some of it might be needed, but most of it will not be used. For example, the lookup processing for the entry point is not needed for unit testing.

May be, it will be better to create several variants of MainEntry and some hierarchy of classes and interfaces around it?

We will need a specific idea of the code from StandardServer.standardEntry() which needs to be shared.

Can you elaborate on this please. I cannot understand what do you mean here.

I was trying to add another remote object interface, like I did for dxgrid before, and I get stuck.

1. I created the interface class UnitTestExports as follows:

package com.goldencode.p2j.client;
public interface UnitTestExports
{
   public void sayHello();
}

package com.goldencode.p2j.client;

public class UnitTestServer
{
   public static void sayHello()
   {
      System.err.println("Hello!");
   }
}

I've added the registration code to the server to StandardServer.bootstrap():

      RemoteObject.registerStaticNetworkServer(new Class[] { UnitTestExports.class }, UnitTestServer.class);

This code is executed successfully, when the server starts.

In ClientCore.start() I am trying to use the interface right before the main.standardEntry() line:

         UnitTestExports unitTestSupport = (UnitTestExports) RemoteObject.obtainNetworkInstance(UnitTestExports.class, session);       
         unitTestSupport.sayHello();

and I got the error:

Caused by: java.lang.RuntimeException: No export or no access to com.goldencode.p2j.client.UnitTestExports:public abstract void com.goldencode.p2j.client.UnitTestExports.sayHello()

Vladimir Tsichevski wrote:

and I got the error:

[...]

From experiments I learned that some existing remote interfaces initialized in StandardServer.bootstrap() do work (e.g. ErrorManager), and some don't (BatchProcess). Why it this? I cannot spot the difference. What am I missing here?

I mean, if we replace the execution of a method of main by execution of an arbitrary lambda function, the access to the contents of the main object will be lost: we will not be able to access it from the lambda.

main is just a proxy object to make calls to the server-side implementation of MainEntry. MainEntry is only needed for:

• some exchange of configuration data and an optional "project token" assignment (this will still be used)
• the execution of the core 4GL "startup program" (some business logic) for the client session via standardEntry() (this will be bypassed and has no meaning in the unit testing approach)

There is no state needed which is managed by the standardEntry() so I don't think this is an issue.

We will need a specific idea of the code from StandardServer.standardEntry() which needs to be shared.

Can you elaborate on this please. I cannot understand what do you mean here.

Inside StandardServer.standardEntry() there is some code that manages the execution of the startup procedure. Some of that code might also be useful/necessary for invoking unit tests, which are converted 4GL programs. If that is the case, then we might want to refactor the common code from StandardServer.standardEntry() into some utility to promote reuse. That is the only reason I could see for some changes there. Otherwise I don't think we need major changes to the MainEntry implementation.

And the implementation class UnitTestServer

UnitTestServer should not be in package com.goldencode.p2j.client. Please place it in com.goldencode.p2j.testengine.

I've added the registration code to the server to StandardServer.bootstrap():

The problem is how you are registering the code. If you look at RemoteObject, you will see there are 2 forms of registration:

• static (registerStaticNetworkServer()) - used when there is a singleton "server" class that "implements" the proxied interface as static methods
• instance (registerNetworkServer()) - used when there is a per-session instance of a server and the instance's backing class actually implements the proxied interface

You are implementing the interface as an "instance server" but registering it as a "static server". That cannot work.

You should read about the remote object approach if you have not already done so.

From experiments I learned that some existing remote interfaces initialized in StandardServer.bootstrap() do work (e.g. ErrorManager), and some don't (BatchProcess).

I don't understand what you mean by this.

Greg Shah wrote:

And the implementation class UnitTestServer

UnitTestServer should not be in package com.goldencode.p2j.client. Please place it in com.goldencode.p2j.testengine.

Done. This have not cured the problem though :-(

I've added the registration code to the server to StandardServer.bootstrap():

The problem is how you are registering the code. If you look at RemoteObject, you will see there are 2 forms of registration:

• static (registerStaticNetworkServer()) - used when there is a singleton "server" class that "implements" the proxied interface as static methods
• instance (registerNetworkServer()) - used when there is a per-session instance of a server and the instance's backing class actually implements the proxied interface

You are implementing the interface as an "instance server" but registering it as a "static server". That cannot work.

I used static implementation methods only. From debugging I found the program has found the implementation method for an interface method.

From experiments I learned that some existing remote interfaces initialized in StandardServer.bootstrap() do work (e.g. ErrorManager), and some don't (BatchProcess).

I don't understand what you mean by this.

I mean I've got the same error when I tried to call some existing remote object class instance, which was not implemented and registered by me.

Please post your code here.

Please look in your directory and search on RemoteErrorData to see the ACL you must have to ensure access to the newly added unit testing remote interface.

Also, please rename UnitTestExports to UnitTestEngine and move it into the testengine package.

Code Review junit5-custom-engine.zip

Overall, I don't have an objection to the code so far. I understand it is early and does not have real integration with executing FWD code, so I think the real implementation will require more discussion.

I assume you are going to eliminate the Maven project structure/cfg and handle the coding standards.

What is the purpose of src/main/resources/META-INF/services/org.junit.platform.engine.TestEngine?

Code Review strict-junit5.diff

I have no objections except to note that history entries are needed.

Code Review uast-hints-unit-test-type.diff

I don't see any purpose in having ELEM_UNITTESTTYPE and also the UNITTESTTYPE that has the same value. Just use ELEM_UNITTESTTYPE everywhere.

Please set a default for unitTestType. It should always be non-null. I don't see a good reason to force each project to have configuration. I think ABLUnit is more commonly used than OEUnit, so make ABLUnit the default.

Greg Shah wrote:

Code Review junit5-custom-engine.zip

Overall, I don't have an objection to the code so far. I understand it is early and does not have real integration with executing FWD code, so I think the real implementation will require more discussion.

Yes, as I said before, this implementation has no connection with FWD. It is rather an universal testing engine with a restricted feature set.

I assume you are going to eliminate the Maven project structure/cfg and handle the coding standards.

Of course yes. It was much faster to develop the code as a standalone project.

What is the purpose of src/main/resources/META-INF/services/org.junit.platform.engine.TestEngine?

It is the standard way the JUni5 framework detect which engines are available. Unfortunately, I do not know how to disable any engine if it is advertise itself with this mechanism, so in Eclipse (which has JUnit5 Jupiter engine as built-in), both engines are used, so I have the same tests executed twice.

Greg Shah wrote:

Code Review strict-junit5.diff

I have no objections except to note that history entries are needed.

Is it a proper time to start committing into 3821c?

No, not yet. I'm hoping to open this up on Thursday.

Before any commit happens, I will need to see the proposed diff that is designed for FWD. You will also need to plan for the changes needed to customer projects that include unit tests.

Greg Shah wrote:

No, not yet. I'm hoping to open this up on Thursday.

Before any commit happens, I will need to see the proposed diff that is designed for FWD. You will also need to plan for the changes needed to customer projects that include unit tests.

Got it.

Greg Shah wrote:

Please look in your directory and search on RemoteErrorData to see the ACL you must have to ensure access to the newly added unit testing remote interface.

Yes! I completely forgot to do this. Now it works, thank you!

Also, please rename UnitTestExports to UnitTestEngine and move it into the testengine package.

Done.

Greg Shah wrote:

Code Review uast-hints-unit-test-type.diff

I don't see any purpose in having ELEM_UNITTESTTYPE and also the UNITTESTTYPE that has the same value. Just use ELEM_UNITTESTTYPE everywhere.

Correct, will fix this.

Please set a default for unitTestType. It should always be non-null. I don't see a good reason to force each project to have configuration. I think ABLUnit is more commonly used than OEUnit, so make ABLUnit the default.

Will do this either. But I myself prefer no default value in cases like this. IMO, it takes less effort to set the proper value in case you forgot to do this, after the system stops conversion with a meaningful error message, than let the system silently proceed in a wrong way.

Vladimir Tsichevski wrote:

The little problem with Eclipse is that it always uses some version of the "Jupiter" engine, which is built into the Eclipse package itself, even this engine is not included into the project classpath. So, every test is executed twice. The good thing is that it is easy to compare the outputs of both engines.

Seems, this is not as little problem as it seemed. The Jupiter engine discovers our tests, attempts to execute them directly, and this crashes (as expected) and aborts the testing.

So we need to either disable the Jupiter engine somehow (which I failed to find out how), or use our own markup for tests (which is the preferable way it seems).

Vladimir Tsichevski wrote:

Vladimir Tsichevski wrote:

The little problem with Eclipse is that it always uses some version of the "Jupiter" engine, which is built into the Eclipse package itself, even this engine is not included into the project classpath. So, every test is executed twice. The good thing is that it is easy to compare the outputs of both engines.

Seems, this is not as little problem as it seemed. The Jupiter engine discovers our tests, attempts to execute them directly, and this crashes (as expected) and aborts the testing.

So we need to either disable the Jupiter engine somehow (which I failed to find out how), or use our own markup for tests (which is the preferable way it seems).

I've created a new package com.goldencode.p2j.testengine.api with a set of annotation classes in it: @After, @AfterAll, @Before, @BeforeAll, @Test. The conversion code is also updated. Have no tried the result yet.

So we need to either disable the Jupiter engine somehow (which I failed to find out how), or use our own markup for tests (which is the preferable way it seems).

I've just read through an example at Deep Dive into JUnit 5 Extension Model. That code shows how to implement custom annotations but this seems to be an additional extension feature that is independent of the custom test engine approach.

I've created a new package com.goldencode.p2j.testengine.api with a set of annotation classes in it: @After, @AfterAll, @Before, @BeforeAll, @Test.

OK, this sounds reasonable. Are you using the @ExtendWith feature for this? I very much like the idea that the code would read as much like normal JUnit tests. My only concern would be if there are negative implications that might occur if one also imports packages directly from JUnit.

Thinking about this, I wonder if we really should implement a different subpackage for com.goldencode.p2j.testengine.oeunit and one for com.goldencode.p2j.testengine.ablunit. These could hold the custom annotations classes and also the assertion classes or anything else that needs to be customized.

Greg Shah wrote:

So we need to either disable the Jupiter engine somehow (which I failed to find out how), or use our own markup for tests (which is the preferable way it seems).

I've just read through an example at Deep Dive into JUnit 5 Extension Model. That code shows how to implement custom annotations but this seems to be an additional extension feature that is independent of the custom test engine approach.

Yes, we will create the custom test engine, because:

1. This approach is explicitly recommended and fully supported by JUnit5 creators
2. The default Jupiter engine is very complicate, and we do not need 95% of it
3. The custom test engine I wrote so far is tiny, but it does everything we need from the front-end side: is supports all the annotation subset we have functionally in 4gl and in FWD unit test classes at the moment. It also have natural and obvious point to change to support our specific need (remote test discovering and execution in FWD).
4. If would be much more unnatural and took much more efforts to try to bend the Jupiter engine to do the same. And, we are not even sure yet if it is possible in the end.

I've created a new package com.goldencode.p2j.testengine.api with a set of annotation classes in it: @After, @AfterAll, @Before, @BeforeAll, @Test.

OK, this sounds reasonable. Are you using the @ExtendWith feature for this?

No, nothing like this is needed. Note: we are creating a test engine from the scratch, so we will implement everything we need directly, we do not need anything to "extend".

I very much like the idea that the code would read as much like normal JUnit tests.

Exactly so. Event more, we can support both kinds of test: the 4gl tests, which require a special treatment, and are marked with "our" special annotations, and the "ordinary" Java tests marked with the usual Jupiter API annotations.

My only concern would be if there are negative implications that might occur if one also imports packages directly from JUnit.

In the converted code, there will be only "our" annotations from the FWD library.

Thinking about this, I wonder if we really should implement a different subpackage for com.goldencode.p2j.testengine.oeunit and one for com.goldencode.p2j.testengine.ablunit. These could hold the custom annotations classes and also the assertion classes or anything else that needs to be customized.

May be, yes, may be, no. I cannot answer this question yet. It will be clear later.

As list of pending changes as noted by Vladimir via email:

  change #1: custom test engine annotations.

  src/com/goldencode/p2j/testengine/api/After.java
  src/com/goldencode/p2j/testengine/api/AfterAll.java
  src/com/goldencode/p2j/testengine/api/Before.java
  src/com/goldencode/p2j/testengine/api/BeforeAll.java
  src/com/goldencode/p2j/testengine/api/Test.java

  change #2: the test engine POC. Does not have to be committed right now, but committing it should not cause any regressions. Depends on: change #1.

  src/com/goldencode/p2j/testengine/FWDClassTestDescriptor.java
  src/com/goldencode/p2j/testengine/FWDEngineDescriptor.java
  src/com/goldencode/p2j/testengine/FWDEngineExecutionContext.java
  src/com/goldencode/p2j/testengine/FWDMethodTestDescriptor.java
  src/com/goldencode/p2j/testengine/FWDMethodTestSource.java
  src/com/goldencode/p2j/testengine/FWDTestEngine.java

  change #3: legacy annotation conversion support.  Depends on: change #1.

  src/com/goldencode/p2j/cfg/Configuration.java
  rules/annotations/annotations.xml
  rules/annotations/legacy_services.rules
  rules/annotations/unit_test_support.rules
  rules/convert/core_conversion.xml
  rules/convert/unit_test_support.rules

  change #4. Refactor FWD test to make them pure JUnit5.

  test/com/goldencode/p2j/comauto/TestComObject.java
  test/com/goldencode/p2j/testing/AbstractFWDTest.java
  test/com/goldencode/p2j/testing/FormatTestSuite.java
test/com/goldencode/p2j/ui/client/format/TestDatetimeFormatParsing.java
test/com/goldencode/p2j/ui/client/format/TestStringFormatParse.java
  test/com/goldencode/p2j/util/AbstractCharacterFormatTest.java
  test/com/goldencode/p2j/util/AbstractDateFormatTest.java
  test/com/goldencode/p2j/util/AbstractDatetimeFormatTest.java
  test/com/goldencode/p2j/util/AbstractLogicalFormatParseTest.java
  test/com/goldencode/p2j/util/AbstractNumberFormatParseTest.java
  test/com/goldencode/p2j/util/AbstractNumberFormatterTest.java
  test/com/goldencode/p2j/util/TestCharacterFormatParser.java
test/com/goldencode/p2j/util/TestCharacterPreprocessFormatString.java
  test/com/goldencode/p2j/util/TestCharacterToStringFormatted.java
  test/com/goldencode/p2j/util/TestCharacterValueOf.java
  test/com/goldencode/p2j/util/TestDateFormatParse.java
  test/com/goldencode/p2j/util/TestDateFormatter.java
  test/com/goldencode/p2j/util/TestDateFormatting.java
  test/com/goldencode/p2j/util/TestDatetimeFormatting.java
  test/com/goldencode/p2j/util/TestLogicalSplitformat.java
  test/com/goldencode/p2j/util/TestNumberTypeParseFormat.java
  test/com/goldencode/p2j/util/TestRowid.java
  test/com/goldencode/util/TestPrintable.java

  change #5. WIP at early stage: calling using the custom engine in FWD through remote protocol

  src/com/goldencode/p2j/main/UnitTestDriver.java
  src/com/goldencode/p2j/main/ClientCore.java
  src/com/goldencode/p2j/main/StandardServer.java
  src/com/goldencode/p2j/testengine/UnitTestEngine.java
  src/com/goldencode/p2j/testengine/UnitTestServer.java

For changes 1 through 4, I would like to see a single patch file that is ready for commit and can be reviewed. While that is being reviewed, VVT can prepare the configuration/scripting/whatever changes needed for the customer projects which use unit testing. VVT should test conversion/compilation of those projects to confirm the safety of the patch.

I think, we can use the BootstrapConfig to pass the test engine discover request data from test runner (e.g. Eclipse or console test runner) up to the MainEntry.

Vladimir Tsichevski wrote:

I think, we can use the BootstrapConfig to pass the test engine discover request data from test runner (e.g. Eclipse or console test runner) up to the MainEntry.

Test discovery request in BootstrapConfig may be encoded like this:

  <node type="test">
     <request class="com.test3820.oo.SimpleTestAblUnit"/>
     <request package="com.test3820.oo"/>
     <request method="com.test3820.oo.SimpleTestAblUnit#someMethod(int arg1)"/>
  </node>

(XML here is used for clarity only)

I have no objection to the bootstrap cfg approach.

To pass test descriptors over network, we need to make them Serializable or Externalizable. Original POS test descriptors based on org.junit.platform.engine.support.descriptor.AbstractTestDescriptor, are not, so we need to re-implement all them from scratch.

I see some problem here.

The JUnit5 testing life cycle assumes the program is being called from outside multiple times: one time to discover and collect tests, next to execute the tests.

We supposed to use ClientCore.start() somehow, but this method does not support this execution model: the client execution is either shut down after the return from main.standardEntry(), or the process execution remains in the internal loop.

As part of my suggestion in #6237-61, I was assuming that the test engine client would "Define the lambda as a executor that processes jobs posted to a queue.". In other words, the client would stay around and process any number of requests posted into some job queue. ClientCore.start() would not return until the test "session" was terminated by the JUnit side.

Is there a problem with this idea?

Greg Shah wrote:

As part of my suggestion in #6237-61, I was assuming that the test engine client would "Define the lambda as a executor that processes jobs posted to a queue.". In other words, the client would stay around and process any number of requests posted into some job queue. ClientCore.start() would not return until the test "session" was terminated by the JUnit side.

Is there a problem with this idea?

After each operation the control should be returned to the test runner software, which orchestrate the process, and which is outside of our control.

Yes, I assumed as much. It is exactly why I suggested that we create a new thread to execute ClientCore.start() instead of using the calling thread (which might be main(). As long as all new requests are posted to the job queue from the test runner, it should work fine.

I suppose FWD expects that everything is executed in some top-level 4gl procedure. In our case we have no such procedure, only 4gl classes and their methods.

Correct. That top-level session setup is in StandardServer.invoke(int, Isolatable). Depending on how OEUnit and ABLUnit work, we can reuse it for each test OR we can call it once and have the code call down to the client in a loop to pick up the next job.

The key question that must be answered about OEUnit and ABLUnit is whether each test or each test set or each test run is a new "session" in 4GL terms. Each session is the execution of a single "startup procedure". There is some cost associated with initializing the session and there is state involved, so we must exactly match the session concept of OEUnit and ABLUnit. We need to check each of the projects independently and match the behavior of each one depending on the test type.

Marian: Do you have any insight into how these work?

At least, OEUnit discovers test recursively (when a test class is a test suite, references classes may be test suites either).
Currently FWD engine does not support this. Need to add the support and test programs.

OEUnit has two more annotations: DataProvider and Fixture. We probably need learn their meaning and add the support.

OEUnit runs the after tests methods even when the before test methods and tests methods sequence ended prematurely with an exception, using the TRY/FINALLY block. We need to compare this behavior with that of JUnit and ABLUnit and change the FWD implementation if necessary.

In OEUnit test methods annotated with @Ignore, are not executed, but a test result with the status Ignored is generated for them anyway.

Currently such methods are completely ignored in FWD.

We need to compare this behavior with that in JUnit and ABLUnit and change FWD implementation if necessary and possible.

Greg Shah wrote:

The key question that must be answered about OEUnit and ABLUnit is whether each test or each test set or each test run is a new "session" in 4GL terms.

Seems OEUnit runs all tests in one session. At least I have not seen anything suspicious in the code.

What is does is:

1. The user passes a file path as argument to the program.
2. If the file path points to a file with .cls extension, this file is assumed to be an OE class source file. The file is compiled and processed.
3. If the file path points to a directory, this directory is searched for OE class source files recursively, and each matching file is processed immediately.

So, unlike JUnit, in OEUnit there is no "test discovery" phase.

OEUnit supports dynamic test suites, for example:

ROUTINE-LEVEL ON ERROR UNDO, THROW.

USING OEUnit.Runner.TestSuite.
USING OEUnit.Runners.RunTest.

CLASS OEUnit.Tests.AllTestSuite INHERITS TestSuite:

  CONSTRUCTOR AllTestSuite():
    AddTest(NEW OEUnit.Tests.Assertion.AllTestSuite()).
    AddTest(NEW OEUnit.Tests.Data.AllTestSuite()).
    AddTest(NEW OEUnit.Tests.Reflection.AllTestSuite()).
    AddTest(NEW OEUnit.Tests.Runner.AllTestSuite()).
    AddTest(NEW OEUnit.Tests.Runners.AllTestSuite()).
    AddTest(NEW OEUnit.Tests.Util.AllTestSuite()).
  END CONSTRUCTOR.

END CLASS.

I think, we will not support these, instead the declarative approach will be used, so the same class will look like this:

ROUTINE-LEVEL ON ERROR UNDO, THROW.

@Suite(value="OEUnit.Tests.Assertion.AllTestSuite,
              OEUnit.Tests.Data.AllTestSuite,
              OEUnit.Tests.Reflection.AllTestSuite,
              OEUnit.Tests.Runner.AllTestSuite,
              OEUnit.Tests.Runners.AllTestSuite,
              OEUnit.Tests.Util.AllTestSuite")
CLASS OEUnit.Tests.AllTestSuite:
  // Empty
END CLASS.

FWD will convert this into something like this:

package oeunit.tests;

import com.goldencode.p2j.testengine.api.*;

@Suite
@SelectClasses(value = {
         OEUnit.Tests.Assertion.AllTestSuite.class,
         OEUnit.Tests.Data.AllTestSuite.class,
         OEUnit.Tests.Reflection.AllTestSuite.class,
         OEUnit.Tests.Runner.AllTestSuite.class,
         OEUnit.Tests.Runners.AllTestSuite.class,
         OEUnit.Tests.Util.AllTestSuite.class})
public class AllTestSuite
{
   // Empty
}

I think, we will not support these, instead the declarative approach will be used, so the same class will look like this:

To make this decision, we must first know any impact on the customer project. If they don't use this feature or if they agree that their usage can be rewritten then we can make this decision.

Otherwise we may have to consider how to support the requirement.

Greg Shah wrote:

I think, we will not support these, instead the declarative approach will be used, so the same class will look like this:

To make this decision, we must first know any impact on the customer project. If they don't use this feature or if they agree that their usage can be rewritten then we can make this decision.

Otherwise we may have to consider how to support the requirement.

Yes. Can we pass this proposal to the customers?

Test inheritance is implemented in OEUnit: not only the methods in the class are discovered, but also methods in superclasses.

For example, the superclass methods marked as @BeforeClass, are executed before the subclass methods marked as @BeforeClass etc.

We need to implement test inheritance either.

Annotations conversion bug found: wrong Java annotation generated: @BeforeAll instead of @BeforeEach and @AfterAll instead of @AfterEach.

How to reproduce: convert two (similar) test classes instead of one, the problem arises for one of them.

Vladimir Tsichevski wrote:

Annotations conversion bug found: wrong Java annotation generated: @BeforeAll instead of @BeforeEach and @AfterAll instead of @AfterEach.

How to reproduce: convert two (similar) test classes instead of one, the problem arises for one of them.

UPD: false alarm: I forget to configure the new class file as being OEUnit type.

I think, we must check that the method type (is method instance or static) must be compatible with the annotation.

Current implementation was developed in assumption that the discover method of a test engine is called by the test runner only once.

It is not the case: if I run the whole project as "JUnit Test" from Eclipse for example, the discover is called for every Java package in the project.

Currently this breaks the program execution and needs to be fixed.

Vladimir Tsichevski wrote:

Greg Shah wrote:

I think, we will not support these, instead the declarative approach will be used, so the same class will look like this:

To make this decision, we must first know any impact on the customer project. If they don't use this feature or if they agree that their usage can be rewritten then we can make this decision.

Otherwise we may have to consider how to support the requirement.

Yes. Can we pass this proposal to the customers?

What is the savings in our time? Why not just implement the support to edit test suites?

Greg Shah wrote:

Vladimir Tsichevski wrote:

Greg Shah wrote:

I think, we will not support these, instead the declarative approach will be used, so the same class will look like this:

To make this decision, we must first know any impact on the customer project. If they don't use this feature or if they agree that their usage can be rewritten then we can make this decision.

Otherwise we may have to consider how to support the requirement.

Yes. Can we pass this proposal to the customers?

What is the savings in our time? Why not just implement the support to edit test suites?

We will need to support dynamic tests then: the tests which are created in runtime by executing customer 4gl code. And this is quite a different thing.

We will need to support dynamic tests then: the tests which are created in runtime by executing customer 4gl code. And this is quite a different thing.

Yes, I understand. Please estimate the impact on our work. What is the cost in time? Is there some other problem it presents?

Greg Shah wrote:

We will need to support dynamic tests then: the tests which are created in runtime by executing customer 4gl code. And this is quite a different thing.

Yes, I understand. Please estimate the impact on our work. What is the cost in time? Is there some other problem it presents?

I can easily estimate work and time implementing the declarative annotation-based approach, and it is quite moderate. With dynamic test, it is harder for now.

Think about it and try to estimate the changes that are needed.

Greg Shah wrote:

Think about it and try to estimate the changes that are needed.

OK

Another problem to solve related to test method discovery:

1. the user requests running a Java package as JUnit Test
2. the ReflectionSupport.findAllClassesInPackage(..) JUni5 utility function is currently used by FWD on the server to collect the test class candidates.

The problem is that method scans the package recursively. So, if several packages are listed as the test discovery input, including base packages and sub-packages, the same classes will be found more than once, and as the result, some test will be executed several times.

This situation occurs, for example, when the user runs the whole Eclipse project as JUnit Test from Eclipse.

I see these ways to resolve this:

1. Collect class test descriptors as a unique set
2. Modify the package discovery, so no recursion is used when searching for classes.
3. Modify the package discovery, so all the classes in sub-packages are filtered out.

The first method is not possible, since there is no such place in the custom test engine code, where all root test descriptors are seen at once.
The second variant is not too convenient, since there is no ready-to-use support for non-recursive package scanning in JUnit5 library.

So I will implement the third method.

The first method is not possible, since there is no such place in the custom test engine code, where all root test descriptors are seen at once.

I don't understand. We control the implementation of the test engine and could certainly maintain persistent state. What am I missing here?

Greg Shah wrote:

The first method is not possible, since there is no such place in the custom test engine code, where all root test descriptors are seen at once.

I don't understand. We control the implementation of the test engine and could certainly maintain persistent state. What am I missing here?

The test engine is not the active site. The test runner (Eclipse in our case) may call the discover engine method several times sending different requests (for example, one request for each Java package). The handles each request separately and returns one root test descriptor for each request.
After that the test runner sends a separate test execution request to our engine for each root test descriptor collected.

So, there is no place in the test engine, where all root test descriptors are seen at once, so we could analyze them all.
But this is also not necessary. The ReflectionSupport.findAllClassesInPackage(..) library function has two additional lambda arguments: class filter and class name filter, which can be used to filter out all classes which do not belong to the "base" package.

I have already fixed this problem, only one line of the code needs to be fixed.

From the other side, the same classes can be collected in one test discovery request: for example, the same class may be collected both as part of the package, and as the class belonging to a test suite class from the same package. This situation is handled nicely by JUnit5 software itself: all discovered test descriptors are collected to a set. We just need to provide the correct equal and hash test descriptor methods (which we did already).

So, this problem is only a small annoyance, since we collect classes recursively first, then drop the result of recursion. Generally, this is not a right thing to do. I would recommend the JUnit5 team to add a non-recursive discovery method.

Unlike JUnit5, OEUnit uses a single instance to run all instance-related test methods in the class (JUnit5 by default creates a new instance for each test, which is the right thing). So, we need to match the OEUnit behavior.

I am implementing the procedure-based unit tests for ABLUnit, and I do not know the proper way to create procedure persistent instances from the known procedure Java class in FWD.

I know, I can get the procedure legacy name from the Java class @LegacySignature annotation, then pass it to ControlFlowOps.invokePersistentSet(..), but may be, there is a shorter way?

Vladimir, what exactly are you trying to achieve? Generate Java code? Write Java code by hand?

Direct usage of ControlFlowOps.invoke APIs (from converted code and FWD runtime) should no longer be used. Instead, there is the InvokeConfig class which offers a builder approach to construct the configuration you want to run. For example: new InvokeConfig(<program>).setProcedureHandle(h).run(arguments);.

In regard to legacy name: if you want to generate code, you need to look into the PROCEDURE AST for this.

Constantin Asofiei wrote:

Vladimir, what exactly are you trying to achieve? Generate Java code? Write Java code by hand?

Neither of these.

In runtime I know the Java methods, which are the results of internal procedure conversion.
I have to create a persistent instance of a procedure, then execute a series of internal procedures for this procedure, then destroy the persistent procedure instance.

Direct usage of ControlFlowOps.invoke APIs (from converted code and FWD runtime) should no longer be used. Instead, there is the InvokeConfig class which offers a builder approach to construct the configuration you want to run. For example: new InvokeConfig(<program>).setProcedureHandle(h).run(arguments);.

OK, will try this

In regard to legacy name: if you want to generate code, you need to look into the PROCEDURE AST for this.

The only conversion thing I need is to convert legacy annotations for internal procedures into Java method annotations, and currently I have no trouble doing this.

Constantin Asofiei wrote:

For example: new InvokeConfig(<program>).setProcedureHandle(h).run(arguments);.

I think, the correct code is:

new InvokeConfig(name).persistent().setProcedureHandle(h).run();

Vladimir Tsichevski wrote:

Constantin Asofiei wrote:

Vladimir, what exactly are you trying to achieve? Generate Java code? Write Java code by hand?

Neither of these.

In runtime I know the Java methods, which are the results of internal procedure conversion.

• use SourceNameMapper.convertJavaProg(String) to get the legacy 4GL name of a program file, where the argument is the fully-qualified string name of a java.lang.Class for a converted program.
• look at the LegacySignature.name to get the legacy name of this internal procedure; non-void Java methods will be legacy functions, and not procedures.

Vladimir Tsichevski wrote:

I think, the correct code is:

[...]

Yes, the config needs to be marked 'persistent'.

Question: shall we use legacy names as display names or converted Java class and method names?

Vladimir Tsichevski wrote:

Question: shall we use legacy names as display names or converted Java class and method names?

Please show an example of how the existing reports render. My initial thought is that we may want to show both legacy and converted names, but I need more context to be sure.

For now, keep using the Java names.

Vladimir Tsichevski wrote:

OEUnit supports dynamic test suites, for example:

[...]

I think, we will not support these, instead the declarative approach will be used, so the same class will look like this:

[...]

Instead of inventing this artificial FWD extensions we may just use the ABLUnit @TestSuite annotation for exactly same purpose (which we will support anyway):

@TestSuite(classes="unittests.SimpleTestOEUnit,
                    unittests.SimpleTestOEUnitInherited").
CLASS unittests.Suite:
  // Empty
END CLASS.

FWD test engine instantiation requires a connection to a FWD server. If this connection fails, the engine creation ends with an exception, which aborts the whole test running process. This leave no chances to other test engines (e.g. the "default" Jupiter test engine).

The FWD test engine should be modified, so if a problem with initializing arises, the engine switches to disabled state, so the discovery request will return no tests, and the execution request does nothing.

4gl allows multiple annotations of the same type for the same element. This is used in some customer code to implement ABLUnit test suites. For example, to implement a test suite of two test classes:

@TestSuite(classes="class1").
@TestSuite(classes="class2").
class Some.AllUnitTests: 
end class.

This will be currently be converted to two @Suite annotations and two @SelectClasses Java annotations, which will neither compile not work.

So we shall either deal with this situation somehow, or fix the customer code, so only one instance of @TestSuite will be used:

@TestSuite(classes="class1, class2").

Please fix it in our conversion (generate only one annotation and merge the data).

In ABLUnit it is valid to use instance methods (no static) as "before all" and "after all" behavior. We need to add the support for this.

Vladimir Tsichevski wrote:

In ABLUnit it is valid to use instance methods (no static) as "before all" and "after all" behavior. We need to add the support for this.

It is supported transparently. When a static method is invoked, the instance is just silently ignored.

Problem: if the test method throws a legacy exception, it is now returned to the client, but the legacy exception is not serializable.
So, we need to either:

1. Do all analysis at the server side and produce some serializable replacement to display at the client side.
2. Make it serializable and process at the client side

Although I see the value in this idea, I wonder if using the JUnit exception instead of the legacy exception will have other unintended side effects.

For example, any 4GL code that calls test methods will potentially break when the JUnit exception is thrown. Can't we raise the legacy exception in a compatible way and then wrap it in the JUnit exception once we are outside of the 4GL code?

I don't see any reason that we can't add the serialization to the OO exception classes here. We have to do this in a more general way in #4658, but the exceptions can be handled now.

Greg Shah wrote:

Although I see the value in this idea, I wonder if using the JUnit exception instead of the legacy exception will have other unintended side effects.

For example, any 4GL code that calls test methods will potentially break when the JUnit exception is thrown. Can't we raise the legacy exception in a compatible way and then wrap it in the JUnit exception once we are outside of the 4GL code?

No, I do not propose using Java exceptions instead of legacy exceptions. Instead, at the server our test engine will recognize the test method has thrown the OpenEdge.Core.AssertionFailedException and handle this very exception type specially: extract the data to the JUnit "standard" exception and throw it.

At the client side this will be raised up to the test runner (for example Eclipse), which know how to present it to the user nicely.

I don't see any reason that we can't add the serialization to the OO exception classes here. We have to do this in a more general way in #4658, but the exceptions can be handled now.

Anyway, we should invent a decent method to present the legacy exception to the user, most probably print it to a string. Since there is no need to do this at the client side, we do not need to serialize legacy exceptions over network.

No, I do not propose using Java exceptions instead of legacy exceptions. Instead, at the server our test engine will recognize the test method has thrown the OpenEdge.Core.AssertionFailedException and handle this very exception type specially: extract the data to the JUnit "standard" exception and throw it.

At the client side this will be raised up to the test runner (for example Eclipse), which know how to present it to the user nicely.

The plan is good.

I see some potential problem: the test engine has no access to the command-line parameters, since these parameters are passed to the test launching engine. So, the only option I see is using the client configuration file with the fixed name, like client.xml located in the deploy/client. Will this pose any problem?

From #6237-61 above:

Gather the necessary configuration (a BootstrapConfig instance) to pass to the ClientCore.start(). We would force batch mode for the session (and any other critical values) but some configuration will be needed to make the proper connection to the server. This will be a runtime thing. We need to discuss how this is encoded/passed to the TestEngine. I want to reuse our existing approaches to configuration (e.g. using the boostrap config XML file) unless there is a really good reason to do something else.

Your question:

I see some potential problem: the test engine has no access to the command-line parameters, since these parameters are passed to the test launching engine.

I know you have a way to launch from Eclipse. We also must support direct JUnit invocation (via something like ant, gradle, Jenkins or github. This will be the more important path. What FWD code gets invoked on this "client" side?

So, the only option I see is using the client configuration file with the fixed name, like client.xml located in the deploy/client. Will this pose any problem?

I don't think this is flexible enough to handle the variations we will see. First, help me understand what our client code looks like.

Greg Shah wrote:

From #6237-61 above:

I want to reuse our existing approaches to configuration (e.g. using the boostrap config XML file) unless there is a really good reason to do something else.

I mean exactly this: using this XML file. My point is that the command-line parameters, which override the values defined in this XML, are now unavailable.

Your question:

I know you have a way to launch from Eclipse. We also must support direct JUnit invocation (via something like ant, gradle, Jenkins or github. This will be the more important path.

AFAIK, all these approaches are very similar: they use some test launching framework, which is part of JUnit5 base, and this JUnit5 base deals with test engines.

There is no such thing as "direct JUnit invocation".

So, running tests from "ant, gradle, Jenkins or github" must be supported automatically, test engines know nothing about these. There is nothing special about Eclipse either.

What FWD code gets invoked on this "client" side?

The first time the test engine is called is test engine class default constructor. The FWD test engine default constructor gathers the configuration, connects to the server and call the initialization method on the server side.

Next time, the test engine is contacted, is one or more calls to the discover method, each call returns a test descriptor.
Next, one or more calls to execute follow, one call for one test descriptor.

There is no such thing as "direct JUnit invocation".

I'm talking about command line launching of JUnit.

The first time the test engine is called is test engine class default constructor. The FWD test engine default constructor gathers the configuration, connects to the server and call the initialization method on the server side.

Are you saying there is absolutely no way to ever pass configuration from some launcher to our code? Surely other test engines can have the need for configuration. It seems surprising that there is no provision for this.

Greg Shah wrote:

Are you saying there is absolutely no way to ever pass configuration from some launcher to our code? Surely other test engines can have the need for configuration. It seems surprising that there is no provision for this.

You can pass configuration parameters, which are available at the discovery and execution time. For example, the junit-platform-console-standalone junit5 console test launcher has the following command-line option to define these parameters in the command-line:

      --config=KEY=VALUE     Set a configuration parameter for test discovery and
                               execution. This option can be repeated.

The problem with these parameters are:

1. they are not supported by all kinds of launchers. For example, Eclipse and ant do not support these. Neither, I have found any related info about gradle launcher. In fact, the junit-platform-console-standalone is the only launcher I know which supports this feature.
2. they are not available at the test engine creation time, and there is no any special "initialization" call. They are available in the discovery calls, which are multiple. This problem is not fatal, but makes the code a bit ugly: you ought to initialize everything at the first discovery call and keep the additional "initialized" flag.

Some questions:

• How do we register our test engine with these launchers?
  • Eclipse
  • ant
  • gradle
  • JUnit command line
• Do we get a LauncherDiscoveryRequest as part of the discovery process?

Greg Shah wrote:

Some questions:

• How do we register our test engine with these launchers?
  • Eclipse
  • ant
  • gradle
  • JUnit command line

We don't. Test engines are discovered by JUnit5 base software by scanning the classpath for org.junit.platform.engine.TestEngine resource.
AFAIK, test launchers have nothing to do with this. So any test engine should work with any launcher out-of-the-box.

• Do we get a LauncherDiscoveryRequest as part of the discovery process?

We receive instances of DefaultDiscoveryRequest as parameter to the discover call.

We need to be able to specify the bootstrap configuration path/filename for a given test run. Different test runs in the same launcher might need different bootstrap configurations.

Hard coding to client.xml is not really a solution. How do you find the client.xml? There may be many on a given drive. It isn't like we can even depend upon deploy/<whatever>/ to be there because that is just a convention, not a requirement. Even with that convention, it is perfectly reasonable to have many projects on the same system.

We receive instances of DefaultDiscoveryRequest as parameter to the discover call.

Right, which is a LauncherDiscoveryRequest which is a EngineDiscoveryRequest which has getConfigurationParameters(). The returned ConfigurationParameters instance seems like it can be initialized from the first junit-platform.properties found in the classpath. If we include our own key/value pair in there, that should provide the "hook" we need.

I'm not the biggest fan, but it does seem to be an option. If we can pass an explicit boottrap config pathname as part of some launcher configuration, I think that is much better because it makes the cfg linkage more explicit.

Vladimir Tsichevski wrote:

My proposal is to follow the same way, and let the assertions library, implemented in FWD, to keep the actual and expected values, which will when be used to form an org.opentest4j.AssertionFailedError instance.

Unfortunately, this does not work with FWD, since the org.opentest4j.AssertionFailedError inherits from java.lang.Error, and is not an java.lang.Exception.

Throwables which are not exceptions are wrapped with RuntimeException in com.goldencode.p2j.net.Dispatcher, and the launchers do not try to extract the expected and actual values from it.

Is it possible to either pass the original org.opentest4j.AssertionFailedError object without wrapping it into RuntimeException or unwrap/convert it back to the org.opentest4j.AssertionFailedError?

Unpack and rethrow it in the calling code on the client.

Greg Shah wrote:

We need to be able to specify the bootstrap configuration path/filename for a given test run. Different test runs in the same launcher might need different bootstrap configurations.

Hard coding to client.xml is not really a solution. How do you find the client.xml? There may be many on a given drive. It isn't like we can even depend upon deploy/<whatever>/ to be there because that is just a convention, not a requirement. Even with that convention, it is perfectly reasonable to have many projects on the same system.

We receive instances of DefaultDiscoveryRequest as parameter to the discover call.

Right, which is a LauncherDiscoveryRequest which is a EngineDiscoveryRequest which has getConfigurationParameters(). The returned ConfigurationParameters instance seems like it can be initialized from the first junit-platform.properties found in the classpath. If we include our own key/value pair in there, that should provide the "hook" we need.

I'm not the biggest fan, but it does seem to be an option. If we can pass an explicit boottrap config pathname as part of some launcher configuration, I think that is much better because it makes the cfg linkage more explicit.

OK, will implement this.

We should add support for the classroot selector in the engine. This selector is not used by Eclipse test launcher (Eclipse itself computes all the project packages), but can be used in the console test launcher.

Vladimir Tsichevski wrote:

We should add support for the classroot selector in the engine. This selector is not used by Eclipse test launcher (Eclipse itself computes all the project packages), but can be used in the console test launcher.

Done. The only minor problem I do not know how to handle is the problem with applications with incomplete classpaths. I use ReflectionUtils from JUnit5 base package to scan classes in packages and class root entries. If some class found during the scanning, can not be loaded due to incomplete class hierarchy or missing referenced classes, the scanning aborts with NoClessDefFoundException. As the result, even if I intercept and ignore this exception, no test classes from this package or class root are collected at all.

And here is an example of running the console engine for entire converted project:

./unittests.sh --exclude-engine=junit-jupiter  --exclude-engine=junit-vintage --include-package=com.test3820.unittests. --scan-classpath
exec java -XX:+HeapDumpOnOutOfMemoryError -Djava.library.path=../../p2j/build/lib -classpath /home/vvt/.m2/repository/org/junit/platform/junit-platform-console-standalone/1.8.2/junit-platform-console-standalone-1.8.2.jar:../../build/classes:../../build/lib/test3820.jar:/home/vvt/Projects/dxgrid/bin/main:../../p2j/build/lib/p2j.jar:../../p2j/build/lib/fwdaopltw.jar org.junit.platform.console.ConsoleLauncher --exclude-engine=junit-jupiter --exclude-engine=junit-vintage --include-package=com.test3820.unittests. --scan-classpath
Picked up _JAVA_OPTIONS: -Duser.timezone=GMT+3
SLF4J: Class path contains multiple SLF4J bindings.
SLF4J: Found binding in [jar:file:/home/vvt/p2j/branches/fwd/build/lib/slf4j-jdk14-1.7.5.jar!/org/slf4j/impl/StaticLoggerBinder.class]
SLF4J: Found binding in [jar:file:/home/vvt/p2j/branches/fwd/build/lib/slf4j-simple-1.6.1.jar!/org/slf4j/impl/StaticLoggerBinder.class]
SLF4J: See http://www.slf4j.org/codes.html#multiple_bindings for an explanation.
SLF4J: Actual binding is of type [org.slf4j.impl.JDK14LoggerFactory]
Nov 14, 2022 8:43:52 PM com.goldencode.p2j.ui.client.gui.theme.ThemeManager setTheme
INFO: UI Theme successfully changed to 'windows10'
Nov 14, 2022 8:43:52 PM com.goldencode.p2j.ui.WidgetDescriptorHelper loadFromDirectory
INFO: Loaded widget descriptor class 'com.org.app.grid.DxGridWidgetDescriptor'.
Nov 14, 2022 8:43:52 PM com.goldencode.p2j.ui.chui.ThinClient lambda$initializePost$1
INFO: Registered widget implementation: com.org.app.grid.config.DxGridConfig com.org.app.grid.grid.DxGridGuiImpl.

Thanks for using JUnit! Support its development at https://junit.org/sponsoring

╷
├─ FWD Test ✔
│  ├─ AssertStringTest ✔
│  │  ├─ isNullOrEmpty ✔
│  │  ├─ contains ✔
│  │  ├─ doesNotContain ✔
│  │  └─ isNotNullOrEmpty ✔
│  ├─ AssertTest ✔
│  │  ├─ objectIsNull ✔
│  │  ├─ objectsAreSame ✔
│  │  ├─ nullIsNotTrue ✔
│  │  ├─ simpleTypesAreEqual ✔
│  │  ├─ typeCastNumbersAreEqual ✔
│  │  ├─ objectsAreEqual ✔
│  │  ├─ isTrue ✔
│  │  ├─ nullIsNotFalse ✔
│  │  ├─ characterIsNotNull ✔
│  │  ├─ fails ✔
│  │  ├─ nullsAreEqual ✔
│  │  ├─ typeCastStringsAreEqual ✔
│  │  ├─ simpleTypesAreNotEqual ✔
│  │  ├─ objectsAreNotEqual ✔
│  │  └─ isFalse ✔
│  ├─ FailMessageTest ✔
│  │  ├─ appErrorReturned ✔
│  │  ├─ appErrorMessage ✔
│  │  ├─ failNoMessage ✔
│  │  ├─ returnError_ ✔
│  │  └─ failWithMessage ✔
│  ├─ SimpleTestAblunitProcedure ✔
│  │  └─ test ✔
│  ├─ SimpleTestProcedure ✔
│  │  ├─ test ✔
│  │  └─ test2 ✔
│  ├─ SimpleTestOeunit ✔
│  │  └─ test ✔
│  ├─ SimpleTestOeunitInherited ✔
│  │  ├─ test ✔
│  │  └─ testOverridden ✔
│  ├─ TestAblassert ✔
│  │  ├─ testAssertEqualsStrings ✔
│  │  ├─ testAssertEqualsStringsFailExpected ✔
│  │  └─ testAssertEqualsStringsFail ✘ Expected: qwerty but was: asdf
│  ├─ TestExpectedError ✔
│  │  ├─ testThrowExplicitly ✔
│  │  └─ testAssertEqualsStringsFailExpected ✔
│  └─ SimpleTestAblunit ✔
│     ├─ test ✔
│     └─ test2 ✔
└─ JUnit Platform Suite ✔

Failures (1):
  FWD Test:TestAblassert:testAssertEqualsStringsFail
    => org.opentest4j.AssertionFailedError: Expected: qwerty but was: asdf
       com.goldencode.p2j.testengine.AbstractMethodTestDescriptor.executeImpl(AbstractMethodTestDescriptor.java:210)
       com.goldencode.p2j.testengine.UnitTestServer.execute(UnitTestServer.java:267)
       com.goldencode.p2j.testengine.UnitTestServerMethodAccess.invoke(Unknown Source)
       com.goldencode.p2j.util.MethodInvoker.invoke(MethodInvoker.java:156)
       com.goldencode.p2j.net.Dispatcher.processInbound(Dispatcher.java:787)
       com.goldencode.p2j.net.Conversation.block(Conversation.java:422)
       com.goldencode.p2j.net.Conversation.run(Conversation.java:232)
       java.lang.Thread.run(Thread.java:750)

Test run finished after 4228 ms
[        12 containers found      ]
[         0 containers skipped    ]
[        12 containers started    ]
[         0 containers aborted    ]
[        12 containers successful ]
[         0 containers failed     ]
[        37 tests found           ]
[         0 tests skipped         ]
[        37 tests started         ]
[         0 tests aborted         ]
[        36 tests successful      ]
[         1 tests failed          ]

Seems, OE does not support multiple ABLUnit life cycle unit test procedures, like JUnit5 supports. That means we cannot use more than one of @Before, @Setup, @TearDown, or @After annotations in one class.

Currently FWD supports this, and I wonder whether we should do unsupport :-)

Vladimir Tsichevski wrote:

Seems, OE does not support multiple ABLUnit life cycle unit test procedures.

This holds for life cycle methods either.

Same for OEUnit.

Currently FWD supports this, and I wonder whether we should do unsupport :-)

We definitely need to implement in a compatible manner. I can also see the value in the current approach. We could make the per-procedure/per-method versions a different annotation and use the @Before, @Setup, @TearDown and @After annotations for the compatible behavior.

Or perhaps it makes more sense to implement the 4GL-compatible versions as annotation names which more clearly mapping to the non-JUnit behavior. Something like @BeforeAll, @SetupAll, @TearDownAll, or @AfterAll?

Vladimir Tsichevski wrote:

Seems, OE does not support multiple ABLUnit life cycle unit test procedures, like JUnit5 supports. That means we cannot use more than one of @Before, @Setup, @TearDown, or @After annotations in one class.

Maybe I just misunderstand you but one can definitively use multiple life cycle procedures/methods in OE.

using Progress.Lang.*.

block-level on error undo, throw.

class DoTest:

    @Before.
    method public void setUpBeforeClass(  ):
        message 'before' view-as alert-box.
    end method.

    @Before.
    method public void setUpBeforeClassTwo(  ):
        message 'before two' view-as alert-box.
    end method.

    @Setup.
    method public void setUp(  ):
         message 'setup' view-as alert-box.
    end method.

    @Setup.
    method public void setUpTwo(  ):
        message 'setup two' view-as alert-box.
    end method.  

    @Test.
    method public void someTest():
         message 'test' view-as alert-box.
    end method.
end class.

This is valid, it compiles and when executed by ABLUnit you'll see messages from all 'before/setup' methods.

Marian Edu wrote:

Vladimir Tsichevski wrote:

Seems, OE does not support multiple ABLUnit life cycle unit test procedures, like JUnit5 supports. That means we cannot use more than one of @Before, @Setup, @TearDown, or @After annotations in one class.

Maybe I just misunderstand you but one can definitively use multiple life cycle procedures/methods in OE.

[...]

This is valid, it compiles and when executed by ABLUnit you'll see messages from all 'before/setup' methods.

This does not compile for me, or at least displays warning messages. Which OE version did you use? I tried with 11.6.

Vladimir Tsichevski wrote:

This does not compile for me, or at least displays warning messages. Which OE version did you use? I tried with 11.6.

We're currently on OE 12.0, that was the version we've used for testcases and the initial basic OO implementation. That is probably the reason you see differences between Assert methods in skeleton.

However, when writing testcases for UI components the behaviour is different in CHUI compared to GUI at times so we've used asserts based on SESSION-DISPLAY and then ran the tests in GUI/CHUI. When moving to ABLUnit in OE we still have the option to choose the display mode by selection 'use tty for runtime' check on ABLUnit run configuration in Eclipse, just though to mention that option in case the engine doesn't already support this.

Marian Edu wrote:

Vladimir Tsichevski wrote:

This does not compile for me, or at least displays warning messages. Which OE version did you use? I tried with 11.6.

We're currently on OE 12.0, that was the version we've used for testcases and the initial basic OO implementation. That is probably the reason you see differences between Assert methods in skeleton.

I have no access to the latest version, so I will probably ask you to run my ABLUnit test cases as they are finished against the latest OE version.

I wonder if there is a way to use preprocessor directives to exclude stuff incompatible with the OE version?

However, when writing testcases for UI components the behaviour is different in CHUI compared to GUI at times so we've used asserts based on SESSION-DISPLAY and then ran the tests in GUI/CHUI. When moving to ABLUnit in OE we still have the option to choose the display mode by selection 'use tty for runtime' check on ABLUnit run configuration in Eclipse, just though to mention that option in case the engine doesn't already support this.

The FWD test engine does not support interactive tests yet.

Features unimplementation status:

1. OEUnit assertion library: converts, compiles, all unit tests included on OEUnit package pass. Add it the the PROPATH to use it.
2. TestSuite (dynamic test construction) is not supported. ABLUnit @TestSuite can be used as the replacement.
3. Reporters: the CSV reporter is not supported. JUnit5 provides reporting API and framework, and any kind of reporting can be added if necessary.
4. Dataproviders are not supported (the dataProvider="DataProviderMethod" unit test method annotation attribute). Dataprovider is a legacy method, which contains data records, which are passed to a test method. So, one test method is called several times and provides multiple test results.

   As I see it now, dataproviders can be implemented in FWD.

5. Fixtures are not supported. Fixture is a special kind of dataprovider. It runs the test method in a transaction, loads data into a dataset, runs the test method, and rollback the transaction. It is not noted in documentation, but I gathered, unlike the "ordinary" dataprovider, one "featured" test method produces one test result.

   As I see it now, features can be implemented in FWD (if the datasets are supported).

To implement all these features we must decide where the OEUnit runtime code will go: will we provide a repository for them or implement in Java in the FWD project, like we did for ABLUnit.

Assume that we must include all support directly in FWD, just like with ABLUnit.

Greg Shah wrote:

Assume that we must include all support directly in FWD, just like with ABLUnit.

Should we do this right now? If yes, which features have to be implemented, and in which order?

OEUnit assertion library: converts, compiles, all unit tests included on OEUnit package pass. Add it the the PROPATH to use it.

We need to implement our own replacement for these. As usual, we must not copy either the 4GL code or the converted code for them. Ours must be written from scratch.

TestSuite (dynamic test construction) is not supported. ABLUnit @TestSuite can be used as the replacement.

My understanding is that these are not equivalent, correct? If so, we should expect to implement the proper replacement.

Reporters: the CSV reporter is not supported. JUnit5 provides reporting API and framework, and any kind of reporting can be added if necessary.

I will check with the customer to see if the CSV support is used. Since this is almost a UI capability, we aren't automatically supporting it.

Dataproviders are not supported (the dataProvider="DataProviderMethod" unit test method annotation attribute). Dataprovider is a legacy method, which contains data records, which are passed to a test method. So, one test method is called several times and provides multiple test results.

Plan to support it.

Fixtures are not supported. Fixture is a special kind of dataprovider. It runs the test method in a transaction, loads data into a dataset, runs the test method, and rollback the transaction. It is not noted in documentation, but I gathered, unlike the "ordinary" dataprovider, one "featured" test method produces one test result.

Plan to support it.

To implement all these features we must decide where the OEUnit runtime code will go: will we provide a repository for them or implement in Java in the FWD project, like we did for ABLUnit.

I need estimates of the time necessary for each part.

Certainly, we can plan that the assertion library must be done first as it is part of the core features used in any test. The order of the others is not obvious to me.

Yes, we need to implement this now.

Testing for equality of MEMPTR objects in OEUnit assertion library is incorrect: the STRING(memptr) values are compared, and any MEMPTR value is not equal even to itself.

In ABLUnit comparison of MEMPTR values is special: values are compared by their sizes and hashes.

What are we going to do with that? Use the ABLUnit-style comparison or leave this incorrect comparison as-is?

Testing for equality of MEMPTR objects in OEUnit assertion library is incorrect: the STRING values are compared, and any MEMPTR value is not equal even to itself.

Are you saying this is a bug in the original 4GL OEUnit code? If so, then please do fix it. I see no reason to maintain their bugs.

Greg Shah wrote:

Testing for equality of MEMPTR objects in OEUnit assertion library is incorrect: the STRING values are compared, and any MEMPTR value is not equal even to itself.

Are you saying this is a bug in the original 4GL OEUnit code? If so, then please do fix it. I see no reason to maintain their bugs.

Yes, I'd consider this a bug. But customer code could depend on this behavior. Also, we can devise multiple ways to compare MEMPTR values. For example, we can do it the same way the ABLUnit does.

Vladimir Tsichevski wrote:

Greg Shah wrote:

Testing for equality of MEMPTR objects in OEUnit assertion library is incorrect: the STRING values are compared, and any MEMPTR value is not equal even to itself.

Are you saying this is a bug in the original 4GL OEUnit code? If so, then please do fix it. I see no reason to maintain their bugs.

Yes, I'd consider this a bug. But customer code could depend on this behavior. Also, we can devise multiple ways to compare MEMPTR values. For example, we can do it the same way the ABLUnit does.

If it always fails, then that limits the possible damage. I think customers will prefer to have the actual failing tests reported and no false positives. Just implement the ABLUnit approach unless you have a better idea.

Do you have effort/time estimates for the rest of the work?

Greg Shah wrote:

Do you have effort/time estimates for the rest of the work?

I think, it will take 2-3 days to implement the two missing features.

I have problem parsing JSON to DataProvider. The offending code is:

    CREATE TEMP-TABLE ttData.
    res = ttData:READ-JSON("FILE", path, "EMPTY").

The conversion result is:

         res.assign(ttData.unwrapJsonData().readJson(new SourceData("LONGCHAR", json), new character("EMPTY")));

It throws the error:

      ErrorManager.recordOrShowError(19079, sourceType, "READ-JSON");

where sourceType is LONGCHAR.

According to the code in JsonImport.createTableStructure(), only the JSONOBJECT and JSONARRAY source types are supported.

Is this a bug or not completely implemented feature? Are there any workarounds?

Vladimir,
There are some issues with your code. Looking at it I assume the following definitions:

DEFINE VARIABLE ttdata AS HANDLE.
DEFINE VARIABLE res AS LOGICAL.
DEFINE VARIABLE path AS LONGCHAR.

In this case the converted code would be:

res.assign(ttData.unwrapJsonData().readJson(new SourceData("FILE", path), new character("EMPTY")));

res.assign(ttData.unwrapJsonData().readJson(new SourceData("LONGCHAR", json), new character("EMPTY")));

• if "FILE" is specified as first parameter of READ-JSON the path must be a valid file path to read the JSON string from;
• if "LONGCHAR" is specified as first parameter of READ-JSON the json must be a valid JSON string to be parsed;
• using a "JSONOBJECT" or a "JSONARRAY" require the respective 4GL objects (these are Progress.Json.ObjectModel.JsonObject and Progress.Json.ObjectModel.JsonArray) to be passed as second argument in READ-JSON.

Ovidiu Maxiniuc wrote:

Vladimir,
There are some issues with your code.

The code is not mine. It is the DateProvider.cls from OEUnit, the FromJSON(INPUT json AS LONGCHAR) method, which is:

  METHOD PUBLIC LOGICAL FromJSON(INPUT json AS LONGCHAR):
    DEFINE VARIABLE res AS LOGICAL NO-UNDO.
    IF VALID-HANDLE(ttQuery) THEN DELETE OBJECT ttQuery.
    IF VALID-HANDLE(ttData) THEN DELETE OBJECT ttData.
    CREATE TEMP-TABLE ttData.
    res = ttData:READ-JSON("LONGCHAR", json, "EMPTY").
    CountSize().
    RETURN res.
  END METHOD.

Looking at it I assume the following definitions:
[...]

In this case the converted code would be:
[...]but not[...]

No, the LONGCHAR was used in the original code.

• if "LONGCHAR" is specified as first parameter of READ-JSON the json must be a valid JSON string to be parsed;

It was valid JSON string, but it the error was thrown before any attempt to parse it was made.

• using a "JSONOBJECT" or a "JSONARRAY" require the respective 4GL objects (these are Progress.Json.ObjectModel.JsonObject and Progress.Json.ObjectModel.JsonArray) to be passed as second argument in READ-JSON.

You mean the original OEUnit code is not workable?

I bumped into a problem implementing DataProvider feature.

The problem is that I obtain the legacy data at the discovery time, create one test method descriptor for each record in DataProvider, and have to use this record at the execute phase.

The problem is that at the execute time the object with the data is considered invalid, since it is not registered in wa.objects map. So, I cannot do anything useful with the data.

I suppose the problem origin is that I create the DataProvider record objects in one block and use them in another blocks.

Vladimir Tsichevski wrote:

No, the LONGCHAR was used in the original code.

• if "LONGCHAR" is specified as first parameter of READ-JSON the json must be a valid JSON string to be parsed;

It was valid JSON string, but it the error was thrown before any attempt to parse it was made.

• using a "JSONOBJECT" or a "JSONARRAY" require the respective 4GL objects (these are Progress.Json.ObjectModel.JsonObject and Progress.Json.ObjectModel.JsonArray) to be passed as second argument in READ-JSON.

You mean the original OEUnit code is not workable?

It seems the respective part of READ-JSON was implemented in FWD based on an older 4GL version. Currently, I see the error 19079 is reserved, but not in use (anymore). So probably OEUnit code is correct, but FWD code must be updated to create the table structure from JSON data.

I suppose the problem origin is that I create the DataProvider record objects in one block and use them in another blocks.

Are you talking about OO 4GL objects here?

Greg Shah wrote:

I suppose the problem origin is that I create the DataProvider record objects in one block and use them in another blocks.

Are you talking about OO 4GL objects here?

Yes, exactly. I managed to overcome this problem by extracting and storing the necessary data from the OO 4GL object in one block at the discovery time and using it in another block at the execution time.

Now I am able to run exactly one test method using DataProvider. The tasks remaining:

1. implement running one test for each data provider record
2. test the DataProvider more thoroughly
3. implement the Fixture feature
4. resolve the #6237-240 problem somehow (the DataProvider working depends on it)
5. create a number of unit tests for both features
6. polish all the new source code
7. rebase the 3827a to the latest trunk

In the file convert/unit_test_support.rules the code beautifying feature was implemented: the qualified class names of FWD test engine annotations were replaced by simple names.

Unfortunately this causes troubles when more than one class with the same simple name is referenced in the converted code. In our case, this problem arises when we convert unit test code referencing the DapaProvider 4GL class, and the DataProvider annotation is also used.

Sp, I decided to remove the feature.

Vladimir Tsichevski wrote:

In the file convert/unit_test_support.rules the code beautifying feature was implemented: the qualified class names of FWD test engine annotations were replaced by simple names.

Unfortunately this causes troubles when more than one class with the same simple name is referenced in the converted code. In our case, this problem arises when we convert unit test code referencing the DapaProvider 4GL class, and the DataProvider annotation is also used.

Sp, I decided to remove the feature.

Instead of removing the feature, perhaps we should just make the annotation names less likely to clash.

Greg Shah wrote:

Vladimir Tsichevski wrote:

In the file convert/unit_test_support.rules the code beautifying feature was implemented: the qualified class names of FWD test engine annotations were replaced by simple names.

Unfortunately this causes troubles when more than one class with the same simple name is referenced in the converted code. In our case, this problem arises when we convert unit test code referencing the DataProvider 4GL class, and the DataProvider annotation is also used.

So, I decided to remove the feature.

Instead of removing the feature, perhaps we should just make the annotation names less likely to clash.

We cannot guarantee the customer will never use names like "Test" or "DataProvider" for his OO classes. And, in the case of the clash we have no means to fix or workaround it in the converted code at the moment. So IMO removing this sugar is a better way.

In the future we can add an optional beautifying build stage which will make any Java code more tidy regardless of the code origin.

We only need to qualify things if the code uses a class that is in conflict. Qualifying all the time is messy and we will live with the consequences for a long time because we will always be busy with other objectives and this will be low priority. I prefer to get this correct now.

Greg Shah wrote:

We only need to qualify things if the code uses a class that is in conflict. Qualifying all the time is messy and we will live with the consequences for a long time because we will always be busy with other objectives and this will be low priority. I prefer to get this correct now.

I reverted the change in 3827a and renamed the conflicting Java file, so now imports are organized again.

Fixture feature implemented.

The branch is 6237a, revision 14583.

This branch includes (and relies upon) unpublished fixes from #7193 and #7184.

Fixtures can only created from @TEMP-TABLE@s yet, due to problems with loading datasets from other kinds of sources in FWD (see #7247, #7193).

What else has to be done for this task: implement OEUnit dynamic test suites feature.

• It is based on old trunk revision.
• It is based on 3827a.
• And it is based on some unknown branch fwd.

I'm not sure how well this can be rebased against current trunk. I suggest you make a copy before you start the rebase.

If the rebase fails, you can always create new task branch and cherry pick all the needed changes.

I will do the review after you rebase.

Hynek Cihlar wrote:

Vladimir, there are issues with 6237a.

• It is based on old trunk revision.

May be so

• It is based on 3827a.

May be so either

• And it is based on some unknown branch fwd.

Hmmm, it is the local name of the project. I do not know how this name affected the bzr.

I'm not sure how well this can be rebased against current trunk. I suggest you make a copy before you start the rebase.

If the rebase fails, you can always create new task branch and cherry pick all the needed changes.

I will do the review after you rebase.

I am in the process right now. Hope, it will be completed with no serious problems.

Code review 6237a.

FWDEngineDescriptor doesn't pop the root transaction scope.

In UnitTestServer the NoClassDefFoundError should be logged with higher log level, warning perhaps.

The following also deserves log output I think:

+      catch (final NoClassDefFoundError ncd)
+      {
+         // We get here if the class hierarchy is incomplete.
+         return Collections.emptyList();
+      }

I also checked in a couple of obvious code cosmetics.

Hynek Cihlar wrote:

Code review 6237a.

FWDEngineDescriptor doesn't pop the root transaction scope.

Please, elaborate, what am I missed, and help me to fix this. I am not very proficient in this part of FWD.

In UnitTestServer the NoClassDefFoundError should be logged with higher log level, warning perhaps.

This situation may happen when some classes in the scanned classes, packages or JARs cannot be loaded since some dependent classes are missing in the classpath. There are plenty of such classes in 250+ JARs in FWD classpath, so I am afraid we can overflow the log with information we already know and can do nothing about :-(

At the moment I cannot reproduce the situation with too many log messages though, so I will change the log level to WARNING as you suggested.

The following also deserves log output I think:
[...]

Agreed. I recall there was some reason I did not do this, but I cannot remember now.

I also checked in a couple of obvious code cosmetics.

No objection.

Vladimir Tsichevski wrote:

At the moment I cannot reproduce the situation with too many log messages though, so I will change the log level to WARNING as you suggested.

The following also deserves log output I think:
[...]

Agreed. I recall there was some reason I did not do this, but I cannot remember now.

Committed as 14597.

Vladimir Tsichevski wrote:

Hynek Cihlar wrote:

Code review 6237a.

FWDEngineDescriptor doesn't pop the root transaction scope.

Please, elaborate, what am I missed, and help me to fix this. I am not very proficient in this part of FWD.

You should call TransactionManager.popScope during the opposite lifecycle phase of the class instance which does pushScope.

At the moment I cannot reproduce the situation with too many log messages though, so I will change the log level to WARNING as you suggested.

OK, the high number of entries can be addressed when we hit the condition.

6237a rebased to the latest trunk, #3827-275 and #3827-282 fixed in rev. 14603.

Hynek Cihlar wrote:

You should call TransactionManager.popScope during the opposite lifecycle phase of the class instance which does pushScope.

I've counted the numbers of times the methods TransactionManager.pushScope(..) and TransactionManager.popScope(..) are called. The numbers are the same, so I gather, the popScope() is called properly?

Vladimir Tsichevski wrote:

Hynek Cihlar wrote:

You should call TransactionManager.popScope during the opposite lifecycle phase of the class instance which does pushScope.

I've counted the numbers of times the methods TransactionManager.pushScope(..) and TransactionManager.popScope(..) are called. The numbers are the same, so I gather, the popScope() is called properly?

What is the code location that removes the root scope?

Hynek Cihlar wrote:

FWDEngineDescriptor doesn't pop the root transaction scope.

You should call TransactionManager.popScope during the opposite lifecycle phase of the class instance which does pushScope.

The prepareImpl calls:

      TransactionManager.pushScope("startup", TransactionManager.NO_TRANSACTION, true, true,
               false, false);

the cleanupImpl does the opposite:

      TransactionManager.popScope();

Vladimir Tsichevski wrote:

The prepareImpl calls:

[...]

the cleanupImpl does the opposite:

[...]

Sounds good then.

What else is needed (review, testing, whatever) before this is ready for merge?

Greg Shah wrote:

What else is needed (review, testing, whatever) before this is ready for merge?

1. Review the small changes still the previous review.
2. Add and run tests for importing DataProvider and Fixture from JSON and XML after #7193 and #7247 are complete.
3. Update the docs with examples and possibly other stuff related to OEUnit.

Do 2 and 3 have to be done before we merge?

Greg Shah wrote:

Do 2 and 3 have to be done before we merge?

Not necessary, the merge should not affect anything not related to these two OEUnit features.

Code review 6237a revisions 14602..14607. The changes look good.

Hynek Cihlar wrote:

Code review 6237a revisions 14602..14607. The changes look good.

Just one minor fix in rev. 14608 to review.

Vladimir Tsichevski wrote:

Hynek Cihlar wrote:

Code review 6237a revisions 14602..14607. The changes look good.

Just one minor fix in rev. 14608 to review.

14608 is good. Please rebase and merge to trunk.

Branch 6237a has been merged to trunk at revision 14541 and archived.

Greg Shah wrote:

Please go ahead with the updates to the documentation.

Done: new sections added: OEUnit-related, and engine control flow in detail.

Constantin found that a customer application which uses OEUnit has an issue with the DataProvider name from FWD unit-test support which collides with a 'DataProvider' converted class name. He thinks all unit-test annotations from FWD need to be registered as reserved names.

Constantin: Do you mean that we should detect when there are conflicts and fully qualify the unit-test annotations in that case?

Greg Shah wrote:

Constantin: Do you mean that we should detect when there are conflicts and fully qualify the unit-test annotations in that case?

The simplest way is to register these names in NameConverter.reservedFWD set. But fully-qualifying can work, too (although I don't know what would need to be done in the rules to do this).

Constantin Asofiei wrote:

Greg Shah wrote:

Constantin: Do you mean that we should detect when there are conflicts and fully qualify the unit-test annotations in that case?

The simplest way is to register these names in NameConverter.reservedFWD set. But fully-qualifying can work, too (although I don't know what would need to be done in the rules to do this).

The names of Java annotations were fully qualified first, when they were simple names, when I changed them to fully qualified names due to the problem similar to this, when they were changed back to simple names. It is not hard to change them back to fully qualified :-)

Always making them fully qualified is not good in the long term. It makes the source too hard to read. In the short term it is OK, though it would be better to detect the problem and only full qualify when needed. That will take some additional development, so fully qualify them for now.

I prefer not to treat all of these as reserved as that can quickly get out of hand. Names like DataProvider are going to be very common in application code.

Vladimir, did you manage to solve the DataProvider issue mentioned previously? Because we need this fixed ASAP.

• any configuration file to create, a basic sample and document what else JUnit-related configuration must be passed to it
• java command with its parameters, again, a basic sample and what else JUnit-related arguments can be used
• any additional jars to download (if it can't be attached to the wiki, post an exact download link). In the wiki is mentioned junit-platform-console-standalone-1.9.2.jar but this does not exist in p2j/build/lib.

• is there a way to specify which converted Java classes annotated as tests for JUnit will be ran? Or a base package where to look for these classes?
• you mention --scan-classpath - what does this do? Will it look through the entire application jar for JUnit classes? I assume this parses the bytecode and does not do Class.forName, as any static initializer code will be executed. I ask because we have an app for which the .class files (in the jar) extract to some ~3.5GB. Looking and parsing through the entire source code will be expensive.

Constantin Asofiei wrote:

Vladimir, we need 'start to finish' wiki steps to run the JUnit tests from converted code. This must include:

• any configuration file to create,

JUnit5 does not use configuration files. Since the usual FWD ClientDriver is not used in JUnit5 architecture, the only way to configure FWD client is to use usual FWD client configuration files. The contents of the file has no JUnit5 specifics.

a basic sample and document what else JUnit-related configuration must be passed to it

see above

• java command with its parameters, again, a basic sample and what else JUnit-related arguments can be used

Java command line is only one of multiple ways to run tests. For example, when running tests from Eclipse IDE, different undocumented Eclipse Java command line application is used.

The command-line parameters are documented at the JUnit5 site.

• any additional jars to download (if it can't be attached to the wiki, post an exact download link). In the wiki is mentioned junit-platform-console-standalone-1.9.2.jar but this does not exist in p2j/build/lib.

Thank you. This has to be fixed. The more correct file is junit-platform-console-1.9.0.jar, it is referenced in build.gradle in the fwdAllTest dependencies, but this does no cause it downloaded.

The docs page should be fixed either.

Also, some questions:

• is there a way to specify which converted Java classes annotated as tests for JUnit will be ran? Or a base package where to look for these classes?

Yes. JUnit5 API allows you to select even specific methods to run. But the way end users can access this API depends on the specific runner. JUnit console application allows to form discovery requests using command-line parameters.

Eclipse provides GUI page for this, and is somewhat more restricted.

• you mention --scan-classpath - what does this do? Will it look through the entire application jar for JUnit classes?

Not exactly. This option works with class catalogs, and ignores anything else like JARs.

I assume this parses the bytecode and does not do Class.forName, as any static initializer code will be executed.

No. This option just select classpath elements, which are file system catalogs, or the specific catalog.

Note also, that the test runner program must no try to be "too smart" and interpret the discovery request parameters. It is the test engine responsibility to interpret query data sent by test runner through the JUnut5 framework. But some test runners like Eclipse try to interpret the data and this imposes some problems in our case.

In our case the classes, packages, class roots etc. do exist in FWD server, while the JUnit5 framework and test runners run as FWD client, so test runners know nothing about the FWD server classpath contents, and hence must not use the FWD client classpath in any way to select classes, packages etc. for discovery. Instead, test runners should blindly pass parameters specified by the user with no checking.

Vladimir, assume you are a 4GL developer who knows nothing about JUnit and just wants to run the converted OEUnit tests. This is what I mean by having 'start to finish' commands to run. Use command line for now, and not IDE-specific setup like Eclipse.

Do this after the DataProvider fix.

Constantin Asofiei wrote:

Vladimir, assume you are a 4GL developer who knows nothing about JUnit ...

We shouldn't restrict this to developers only. A sys admin or tester should be able to run unit tests.

Constantin Asofiei wrote:

Vladimir, did you manage to solve the DataProvider issue mentioned previously? Because we need this fixed ASAP.

Which one do you mean?
Is that #6237-240 or #6237-242 or #6237-243?

AFAIK, all issues related to DataProvider are resolved in trunk.

Vladimir Tsichevski wrote:

Constantin Asofiei wrote:

Vladimir, did you manage to solve the DataProvider issue mentioned previously? Because we need this fixed ASAP.

Which one do you mean?
Is that #6237-240 or #6237-242 or #6237-243?

AFAIK, all issues related to DataProvider are resolved in trunk.

There are still collisions between the Java class name for a converted .cls/.p source and DataProvider name. I've converted an app with 7156b, which is using trunk 14622

This customer has this class in conversion:

abl/oeunit/src/OEUnit/Data/DataProvider.cls

Vladimir Tsichevski wrote:

Which one do you mean?

#6237-280

Greg Shah wrote:

Constantin found that a customer application which uses OEUnit has an issue with the DataProvider name from FWD unit-test support which collides with a 'DataProvider' converted class name. He thinks all unit-test annotations from FWD need to be registered as reserved names.

Constantin: Do you mean that we should detect when there are conflicts and fully qualify the unit-test annotations in that case?

What exactly problem occurred? Was the class name for abl/oeunit/src/OEUnit/Data/DataProvider.cls OEUnit.Data.DataProvider or was this oeunit.src.OEUnit.Data.DataProvider. Why does this customer needs to convert the original OEUnit? The results of conversion cannot be used in FWD.

Thanks for this question. Now I recall the original scenario. This app has these dependencies:

oeunit.assertion.assert
oeunit.assertion.assertionfailederror
oeunit.data.dataprovider
oeunit.runners.oeunitrunner
oeunit.runners.runtest
oeunit.runner.testclassresult
oeunit.runner.testmethodresult
oeunit.runner.testresult
oeunit.runner.testsuite

At this point, are all these classes in skeleton OEUnit? If yes, we can just remove the oeunit from abl/ and run conversion again.

Constantin Asofiei wrote:

Thanks for this question. Now I recall the original scenario. This app has these dependencies:
[...]

At this point, are all these classes in skeleton OEUnit? If yes, we can just remove the oeunit from abl/ and run conversion again.

The oeunit.runners.XXX classes except the oeunit.runners.TestSuite are not implemented in FWD, since they are neither needed nor usable. All other referenced classes are implemented and present in the skeleton.

Thanks, for that app, I can remove everything from oeunit except oeunit.runners.oeunitrunner and oeunit.runners.runtest and do another conversion .

Please work on the wiki.

Constantin Asofiei wrote:

Please work on the wiki.

About the instruction I have to write. Shall it be something like "Writing Hello World in FWD ABLUnit" or "ABLUnit to FWD migration guide"?

Vladimir Tsichevski wrote:

About the instruction I have to write. Shall it be something like "Writing Hello World in FWD ABLUnit" or "ABLUnit to FWD migration guide"?

For consistency, I think we need (almost) both. But I don't think we need the conversion steps, only something like: this is a sample unit test in 4GL, it gets converted to this Java class which looks like this, and these are all the steps needed to run this test in FWD. Include examples how to specify package, regexp or other specs to identify and run multiple tests.

Is there a difference between OEUnit and ABLUnit, in terms how FWD runs the tests?

Constantin Asofiei wrote:

Vladimir Tsichevski wrote:

About the instruction I have to write. Shall it be something like "Writing Hello World in FWD ABLUnit" or "ABLUnit to FWD migration guide"?

For consistency, I think we need (almost) both. But I don't think we need the conversion steps, only something like: this is a sample unit test in 4GL, it gets converted to this Java class which looks like this, and these are all the steps needed to run this test in FWD.

These are basically the same steps as for running any FWD application, and AFAIK we have no complete instruction on how to do that :-(

Include examples how to specify package, regexp or other specs to identify and run multiple tests.

OK, this will basically copy-paste from JUnit5 site.

Is there a difference between OEUnit and ABLUnit, in terms how FWD runs the tests?

No difference, except that there is no such things as Fixture and DataProvider in ABLUnit.

A repo 3827c was created for new fixes.
I changed the build.gradle: junit-platform-console JAR was added to the fwdClient dependencies instead of junit-platform-console-standalone. See #6237-285.

The reasons:

1. the junit-platform-console-standalone is uber-jar consisting mostly of parts already included into the dependency list.
2. this junit-platform-console-standalone dependency was listed in the test dependencies, to this JAR was not downloaded and installed by gradle into build/libs anyway.

Still I have to fix the docs page to reflect this change.

Vladimir Tsichevski wrote:

Still I have to fix the docs page to reflect this change.

Done.

As to the new instruction: shall I write a new Redmine page (preferred, someone must add such a page to Redmine for me first), or add a chapter to the 4GL Unit Testing page?

Vladimir Tsichevski wrote:

As to the new instruction: shall I write a new Redmine page...

I already devised the new page name: FWD ABLUnit Primer :-)

As to the new instruction: shall I write a new Redmine page...

I already devised the new page name: FWD ABLUnit Primer :-)

I know it can be nice to write a fresh page, but I prefer a single reference in this case. I've just gone through the 4GL Unit Testing page and made some minor edits (typos/grammar fixed, some formatting changes, added a copyright notice). Pleace add a "Primer" section at the top (just above "Writing Tests") and make sure it addresses both OEUnit and ABLUnit.

Greg Shah wrote:

As to the new instruction: shall I write a new Redmine page...

I already devised the new page name: FWD ABLUnit Primer :-)

Please add a "Primer" section at the top (just above "Writing Tests") and make sure it addresses both OEUnit and ABLUnit.

I think, this section should not cover all aspects of ABLUnit and OEUnit, just a simple example to let the user create, run and view the results only. Otherwise the contents will duplicate the existing contents of the 4GL Unit Testing page. These simple examples are the same for ABLUnit and OEUnit, so there is no point duplicate the code, just to mention the fact.

Vladimir Tsichevski wrote:

Include examples how to specify package, regexp or other specs to identify and run multiple tests.

OK, this will basically copy-paste from JUnit5 site.

While we can quote small (i.e., a sentence or two) passages with proper attribution in order to support a specific point, we cannot copy-paste whole sections of anyone else's documentation into our own. If small quotes won't do, please use references/links instead.

Vladimir Tsichevski wrote:

Greg Shah wrote:

As to the new instruction: shall I write a new Redmine page...

I already devised the new page name: FWD ABLUnit Primer :-)

Please add a "Primer" section at the top (just above "Writing Tests") and make sure it addresses both OEUnit and ABLUnit.

I think, this section should not cover all aspects of ABLUnit and OEUnit, just a simple example to let the user create, run and view the results only. Otherwise the contents will duplicate the existing contents of the 4GL Unit Testing page. These simple examples are the same for ABLUnit and OEUnit, so there is no point duplicate the code, just to mention the fact.

OK, I think that is fine.

The 4GL_Unit_Testing page was updated: the "Primer" section was added.

As to the Java class name clash problem: I think, the only reliable solution would be as follows:

1. conversion always generates qualified names
2. after conversion, Java is post-processed with a beautifier, which replaces fully qualified names with simple names, unless it would cause problems. This stage has nothing to do with FWD and conversion, and can be applied to any Java project.

I don't want to go down this route right now. We have this problem generally and will address it in another way.

For now, please rename the Java class to something unique like _DataProvider_.

Greg Shah wrote:

I don't want to go down this route right now. We have this problem generally and will address it in another way.

For now, please rename the Java class to something unique like _DataProvider_.

Greg, this is not a priority, it can be postponed. The issue with the customer app is solved by just removing this OEUnit file from abl/. There are no other issues with customer apps.

Is there any reason to leave this task open?

Greg Shah wrote:

Is there any reason to leave this task open?

If we are all decided not to do anything with name collision around the DataProvider class, then yes.

Vladimir, a question, please. A customer uses this:

using OEUnit.Assertion.Assert.

      Assert:IsTrue(...).

      Assert:AreEqual..., ...).

OEUnit.Assertion.Assert does not exist in skeleton. Is this supposed to work in FWD? If not, what do we need to fix this?

There are these additional classes referenced by the customer's project:

OEUnit.Runner.TestClassResult
OEUnit.Runners.OEUnitRunner

There may be more once the customer parses the entire application.

Constantin Asofiei wrote:

Vladimir, a question, please. A customer uses this:
[...]

OEUnit.Assertion.Assert does not exist in skeleton.

It does.

Is this supposed to work in FWD?

Yes, of course

If not, what do we need to fix this?

Checkout the skeleton project?

17:55:19:/tmp$ bzr co /home/vvt/secure/code/p2j_repo/skeleton
17:57:12:/tmp$ ls -l skeleton/oo4gl/OEUnit/Assertion/
total 24
-rw-rw---- 1 vvt vvt 13830 Sep  8 17:57 Assert.cls
-rw-rw---- 1 vvt vvt  2677 Sep  8 17:57 AssertionFailedError.cls
-rw-rw---- 1 vvt vvt  1778 Sep  8 17:57 AssertString.cls

Thanks, I missed that somehow. What about the other two classes?

Constantin Asofiei wrote:

There are these additional classes referenced by the customer's project:
[...]

There may be more once the customer parses the entire application.

These are not implemented since the tests are executed by JUnit5, so these classes are not needed (and cannot be used).

Vladimir Tsichevski wrote:

Constantin Asofiei wrote:

There are these additional classes referenced by the customer's project:
[...]

There may be more once the customer parses the entire application.

These are not implemented since the tests are executed by JUnit5, so these classes are not needed (and cannot be used).

OK, I'll check with the customer and see the role of the files using these classes, and if they can be excluded.

Constantin Asofiei wrote:

Vladimir Tsichevski wrote:

Constantin Asofiei wrote:

There are these additional classes referenced by the customer's project:
[...]

There may be more once the customer parses the entire application.

These are not implemented since the tests are executed by JUnit5, so these classes are not needed (and cannot be used).

OK, I'll check with the customer and see the role of the files using these classes, and if they can be excluded.

I think, they use them for reporting and displaying the test results in Eclipse, and we use reporting supplied with JUnit5 and Eclipse own JUnit5 test runner.