FWD » Testing

Marian Edu wrote:

Is this a sub-task of conversion testing?

Ah, I see now there are separate projects for each topic so this just falls under 'conversion' topic.

To be clear: we will implement a driver that directly executes the lexer and checks the output. We must check both valid and invalid cases.

Greg Shah wrote:

To be clear: we will implement a driver that directly executes the lexer and checks the output. We must check both valid and invalid cases.

What exactly should we consider as the 'lexer output'? Is it the ordered token list generated in .lexer files? If so, since this is available only in the context of FWD, what should the 4GL tests be checking? Just the presence/absence of compiler errors? I experimented a bit with the COMPILER system handle and it can be used to retrieve the line(s) and column(s) where an error takes place, together with the message, error code, etc. Here is a sample:

compile test1.p no-error.
message "errors " compiler:error.

message "num messages " compiler:num-messages.
define variable i as integer init 1.
do while i <= compiler:num-messages:
    message "row: " compiler:get-row(i) "~n".
    message "column: " compiler:get-column(i) "~n".
    message "message type: " compiler:get-message-type(i) "~n".
    message "message: " compiler:GET-MESSAGE(i)  "~n".
    message "error code: " compiler:get-number(i)  "~n".
    message "==========================================".
    i = i + 1.
end.

errors  yes
num messages  6
row:  8
column:  5
message type:  1
message:  ** Unable to understand after -- "fst = 0". (247)
error code:  247
==========================================
...
...

In the OE testing, we only need to know if it compiled or not and if it failed to compile, the details (as you have already found). We don't care how they lex or parse something internally.

In FWD, it is different. We must also fail for the same input that would cause a failure in OE. BUT we really do care about the token stream that is generated. If that token stream is wrong, then the lexing should fail testing even if there is no fatal error. This is because our downstream processing (both parsing and TRPL) really depends on the exact tokens that are generated by the lexer.

So I expect the Java driver to be much more complicated. Each 4GL program will need some kind of "baseline" file that persists the token stream in a way we can read in and compare with the one generated by the lexer under test.

I have created the tests/conversion/lexer directory in testcases and added tests for COMMENT rule and for STRING : ( SSTRING | DSTRING ) (STR_OPTIONS)? rule.

I took the liberty and made some initial classes and method definitions for the FWD Driver.

I've created 2 classes:

• LexerTestConfiguration.java, which will store the information of a particular driver execution, such as progress.g specific flags and most importantly the input files and the output location of the generated .lexer files:
  

  public class LexerTestConfiguration
  {
      public LexerTestConfiguration(String[] args)
      {
          // parse the parameters and set the flags/attributes
      }
  
      private List<String> inputs = new ArrayList<>();
      private String outputDir;
  
      private boolean hidden      = false;
      private boolean raw         = false;
      private boolean schema      = false;
      private boolean unixEscapes = false;
      private boolean export      = false;
      private boolean bypassSkip  = false;
  
      // setter + getters
  
      public LexerDumpFilter createLexerForConfiguration(String inputFile)
      throws IOException
      {
          FileReader     fr     = new FileReader(inputFile);
          BufferedReader br     = new BufferedReader(fr);
          SymbolResolver sym    = new SymbolResolver(true);
  
          return new LexerDumpFilter(br, sym, this);
      }
  }
  
  

  • The user will have the option to pass a directory or a single test. In both cases the inputs list will store all the tests (which have been collected recursively).
  • I added a new constructor in LexerDumpFilter.java which makes use of a LexerTestConfiguration object to copy the attribute values.

• The driver class: LexerTestDriver.java:
  

  public class LexerTestDriver
  {
      private static void execute(LexerTestConfiguration lexerTestConfiguration)
      throws IOException, TokenStreamException
      {
          for (String input : lexerTestConfiguration.getInputs())
          {
              LexerDumpFilter lexer = lexerTestConfiguration.createLexerForConfiguration(input);
  
              // lexer.setOutput(...);
              lexer.dump();
          }
      }
  
      public static void main(String[] args)
      {
          LexerTestConfiguration ltc = new LexerTestConfiguration(args);
          try
          {
              execute(ltc);
          }
          catch (Exception e)
          {
  //            ...
          }
      }
  }
   

  • This will create a configuration object and lex the files. For each file a new LexerDumpFilter object is instantiated with the required input. The output file can have the same name and the .lexer extension.

Greg, what do you think of my approach? Do you see any issues?

I have created the tests/conversion/lexer directory in testcases and added tests for COMMENT rule and for STRING : ( SSTRING | DSTRING ) (STR_OPTIONS)? rule.

These are a good start. I like the way you are organizing the cases, the structure and naming is good.

• We will need to expand them to handle a wider range of input inside comments and both kinds of strings. For example:
  • A wider range of input characters, especially control characters and all forms of whitespace. This should not be exclusively in the single-byte character range.
  • Various escape sequences, including octal and unicode escapes that might cause trouble.
  • Both kinds of escape character (backslash when unixEscapes is one and tilde always).
  • Different source encodings.
  • Nested // style comments and (when we have tests for // comments, star comments/strings inside // style comments).
  • Comments in strings and strings in comments.
• We will need to add cases with conflicting content outside of the comments or strings. For example, matching comments after other content like symbols but with no intervening whitespace.
• You plan to encode the FWD output as .lexer files?
• How do you plan to encode the expected failures in the invalid cases?

Note that the escape sequences issue above, is only a possible problem because of the preprocessor implementing conversion from the escaped version to the actual character. Although I don't think we want all of our testcases to be preprocessed, I wonder what to do for the escape cases.

Greg, what do you think of my approach? Do you see any issues?

I like the approach. We will have to see how the inputs are encoded. Instead of creating the LexerTestConfiguration by passing String[] args, I would prefer if the LexerTestConfiguration had a specific parameter list and the driver program itself was respinsible for parsing the inputs and then creating the LexerTestConfiguration. Keep going, this is good.

Thanks for the reviews.

Greg Shah wrote:

You plan to encode the FWD output as .lexer files?

Yes, at least for the successful ones. Note that we can successfully lex procedures in FWD that would otherwise fail compilation in 4GL, I have some examples.

I have implemented most of the logic and will make a commit tomorrow morning. We have to decide on some things in order to finish the implementation:

• Do we need to preprocess the files before we obtain the .lexer files? Should we always do that or only on demand?
• The same question but regarding the schema cvt files generation (such as the standard.dict, if we run the lexer without it but with the schema flag as true for the symbol resolver, some exceptions are thrown. The lexer is done successfully nevertheless).

How do you plan to encode the expected failures in the invalid cases?

• If the lexer fails, the .lexer file can be empty, but not always. In any case some exceptions are thrown and the <EOF> token is probably not generated. We can leave the .lexer as is or we can append some concrete error messages, such as:

line 1:22: expecting '"', found '<EOF>'

These are generated by the lexer.dump().

Yes, at least for the successful ones. Note that we can successfully lex procedures in FWD that would otherwise fail compilation in 4GL, I have some examples.

We will probably want to tighten up the lexing rules so that these fail.

Do we need to preprocess the files before we obtain the .lexer files? Should we always do that or only on demand?

Yes and Yes

The same question but regarding the schema cvt files generation (such as the standard.dict, if we run the lexer without it but with the schema flag as true for the symbol resolver, some exceptions are thrown. The lexer is done successfully nevertheless).

I checked this and it is safe to pass false for the schema parameter when instantiating the SymbolResolver.

How do you plan to encode the expected failures in the invalid cases?

• If the lexer fails, the .lexer file can be empty, but not always. In any case some exceptions are thrown and the <EOF> token is probably not generated. We can leave the .lexer as is or we can append some concrete error messages, such as:

[...]

These are generated by the lexer.dump().

OK. In the shift to ANTLR v4, we will formalize the error handling with more specific and informative errors. Then we can use those in our encoded error processing.

I've made a commit to 6860a/15839 with what I've done so far.

The added classes are

added src/com/goldencode/p2j/uast/InvalidLexerTestCommandException.java
added src/com/goldencode/p2j/uast/LexerTestDriver.java
added src/com/goldencode/p2j/uast/LexerTestOptions.java
added src/com/goldencode/p2j/uast/LexerTestScenario.java

It is a working first version but undocumented. I implemented the command parsing and preprocessing before lexing. The preprocessing saves the preprocessed content into a string buffer which will be the input for the LexerDumpFilter (no .cache are generated). I still have to work on the error handling and formatting. So far, it supports the parameters:

• hidden
• raw
• unixEscapes
• export
• bypassSkip
• maxDepth (for the case when a folder with subfolders is given as input, -1 is the default and it means total recursion)

The user can either supply an input file OR an input directory and the path to the desired output directory. The output directory will respect the file structure of the input directory. Some examples of input parameters:

1.

-unixEscapes
-maxDepth 3
/home/fer/Conversions/dataset/dataset_clean/abl/strings
/home/fer/Conversions/dataset/dataset_clean/abl/baseline

2.

-unixEscapes
-maxDepth 3
/home/fer/Conversions/dataset/dataset_clean/abl/start.p

etc.

I've worked the exception handling and error-logging. I also re-formatted some parts and started working on documenting. So far I have documented the

src/com/goldencode/p2j/uast/LexerTestOptions.java
src/com/goldencode/p2j/uast/LexerTestScenario.java

classes.

I made a commit to 6860a/15840.

Regarding the FWD-OE Harness:

We can have a common Test Plan:

<test-plan name="Lexer Test Plan" description="Lexer Test Plan">
<output-directory path="results" unique="true"/>
   <target type="stream" shell="bash"/>
   <resources> 
        <variable name="run_progress_script" type="String" initial="C:\Progress\OE116_64\bin\_progres.exe"/>
   </resources>
   <test-set dependency="PRE_CONDITION">
   <test-set name="Comments_test_plan" description="Comments test plan" threads="1">   
   <test filename="comments_test.xml"/>
</test-set>
</test-plan>

But the <test> elements are quite different in FWD and in OE. In fwd I use file comparison tests whereas in OE I use <stream-wait-for value="..." millis="1000" newline="false"/>, where value is either yes or no depending on whether the compilation was successful. This is why, without any built-in harness conditionals, we either have to design 2 different test-plans or use an Ant task which wraps the java -jar harness.jar test-plan.xml call.

Greg, what would you suggest?

Example of a test in FWD:

<test name="comments_test" description="comments test">
   <stream-write value="ant -f ../../../build.xml run-lex -Dinput=tests/conversion/lexer/comments -Doutput=tests/conversion/lexer/comments_output" 
                 newline="true"/>
   <stream-wait-for value="BUILD SUCCESSFUL" newline="false" millis="30000" />
   <text-file-comparison baseline="comments_baseline/valid/inline.lexer" actual="comments_output/valid/inline.lexer" />
</test>

Example of a test in OE:

<?xml version="1.0"?>
<test name="comments_test" description="" >
    <stream-write value="%s -p ..\driver.p -param     ..\comment\valid\inline.p -b" newline="true">
        <substitution variable="run_progress_script"/>
    </stream-write>
    <stream-wait-for value="no"  millis="1000" newline="false"/>
</test>

Code Review Task Branch 6860a Revisions 15838 through 15842

It is looking good.

1. Instead of passing printToStdout to the Preprocessor constructor, it is better to add a member to the Options class and set it in the callers when needed.

2. Please put the "test code" into a test-specific sub-package (com.goldencode.p2j.uast.test). By "test code" I mean the files that are specific to testing AND which have no direct references from the common code.

3. The new LexerDumpFilter(Reader in, SymbolResolver sr, LexerTestOptions lto) constructor needs javadoc.

4. LexerTestDriver, progress.g needs a standard header.

5. In LexerTestDriver, the private methods, the inner class and the main() should be at the bottom of the class, in that order.

6. LexerDumpFilter needs a history entry.

But the <test> elements are quite different in FWD and in OE.

If there is no overlap, then we should use separate test .xml files (comments_test_fwd and comments_test_oe).

6860a rev 15843 has some code format cleanup.

Florin: Please review the changes and follow the standards in the future.

Greg Shah wrote:

Code Review Task Branch 6860a Revisions 15838 through 15842

It is looking good.

1. Instead of passing printToStdout to the Preprocessor constructor, it is better to add a member to the Options class and set it in the callers when needed.

2. Please put the "test code" into a test-specific sub-package (com.goldencode.p2j.uast.test). By "test code" I mean the files that are specific to testing AND which have no direct references from the common code.

3. The new LexerDumpFilter(Reader in, SymbolResolver sr, LexerTestOptions lto) constructor needs javadoc.

4. LexerTestDriver, progress.g needs a standard header.

5. In LexerTestDriver, the private methods, the inner class and the main() should be at the bottom of the class, in that order.

6. LexerDumpFilter needs a history entry.

Thanks for the review. I solved the issues and committed to 6860a/15844.

I have committed the OE Harness tests for Lexer. It consists of the driver.p and the oe_test_plan.xml which calls the tests from oe_*_harness_xml. The oe_results has a report example.

The tests for FWD are also committed, but I am trying to improve them to get a better run time using the threads= option, as mentioned in #9854-49.

I made another commit adding all tests we have so far to the FWD Harness.

There is an issue: when I run the FWD Harness, some tests fail due to timeout. This is non-deterministic (each time different tests can fail). I don't think the FWD Driver is at fault, since I tried running it with a script against the all .p files all of them returned BUILD SUCCESSFUL. I tried increasing the timout (currently I am at 2 min) and the number of the failing tests did decrease a bit, but it still doesn't make sense, since the tests shouldn't need a timout that high. I think it's some sort of deadlock inside the harness. I will try running them with a very high timeout to see if we can get all the tests to pass.

The testcases/tests/conversion/lexer/fwd_results contains the reports.

I have run 10 lexers in parallel and discovered that the issue is because of the cvtdb.h2.db2 file which acts like a critical section. Basically all 10 conversions use the same H2 DB.

     [java] Preprocessing file /home/fer/gcd/branches/testcases/tests/conversion/lexer/deadlock_tests/batch1/test1.p: success.
     [java] Unexpected severe error occurred: Error opening database connection
     [java] 
     [java] org.h2.jdbc.JdbcSQLNonTransientConnectionException: Error opening database: "Lock file recently modified" [8000-200]
     [java]     at org.h2.message.DbException.getJdbcSQLException(DbException.java:455)
     [java]     at org.h2.message.DbException.getJdbcSQLException(DbException.java:429)
     [java]     at org.h2.message.DbException.get(DbException.java:194)
     [java]     at org.h2.store.FileLock.getExceptionFatal(FileLock.java:428)
     [java]     at org.h2.store.FileLock.waitUntilOld(FileLock.java:300)
     [java]     at org.h2.store.FileLock.lockFile(FileLock.java:317)
     [java]     at org.h2.store.FileLock.lock(FileLock.java:108)
     [java]     at org.h2.engine.Database.open(Database.java:656)
     [java]     at org.h2.engine.Database.openDatabase(Database.java:307)
     [java]     at org.h2.engine.Database.<init>(Database.java:301)
     [java]     at org.h2.engine.Engine.openSession(Engine.java:74)
     [java]     at org.h2.engine.Engine.openSession(Engine.java:192)
     [java]     at org.h2.engine.Engine.createSessionAndValidate(Engine.java:171)
     [java]     at org.h2.engine.Engine.createSession(Engine.java:166)
     [java]     at org.h2.engine.Engine.createSession(Engine.java:29)
     [java]     at org.h2.engine.SessionRemote.connectEmbeddedOrServer(SessionRemote.java:341)
     [java]     at org.h2.jdbc.JdbcConnection.<init>(JdbcConnection.java:182)
     [java]     at org.h2.jdbc.JdbcConnection.<init>(JdbcConnection.java:161)
     [java]     at org.h2.Driver.connect(Driver.java:69)
     [java]     at java.sql.DriverManager.getConnection(DriverManager.java:664)
     [java]     at java.sql.DriverManager.getConnection(DriverManager.java:247)
     [java]     at com.goldencode.p2j.convert.db.DBHelper.connect(DBHelper.java:243)
     [java]     at com.goldencode.p2j.convert.db.ConversionData.connect(ConversionData.java:279)
     [java]     at com.goldencode.p2j.convert.db.ConversionData.listLegacyClasses(ConversionData.java:589)
     [java]     at com.goldencode.p2j.uast.SymbolResolver.loadLegacyClasses(SymbolResolver.java:2243)
     [java]     at com.goldencode.p2j.uast.SymbolResolver.<init>(SymbolResolver.java:1244)
     [java]     at com.goldencode.p2j.uast.SymbolResolver.<init>(SymbolResolver.java:1133)
     [java]     at com.goldencode.p2j.uast.test.LexerTestScenario.createLexerForScenario(LexerTestScenario.java:166)
     [java]     at com.goldencode.p2j.uast.test.LexerTestDriver.execute(LexerTestDriver.java:227)
     [java]     at com.goldencode.p2j.uast.test.LexerTestDriver.main(LexerTestDriver.java:129)

One solution would be to have a dedicated cvt folder for each testing conversion.

Florin Eugen Rotaru wrote:

One solution would be to have a dedicated cvt folder for each testing conversion.

Alternatively, we can pass the dict=false to the SymbolResolver constructor, this will not call loadLegacyClasses().

One solution would be to have a dedicated cvt folder for each testing conversion.

We must not do this.

Florin Eugen Rotaru wrote:

Florin Eugen Rotaru wrote:

One solution would be to have a dedicated cvt folder for each testing conversion.

Alternatively, we can pass the dict=false to the SymbolResolver constructor, this will not call loadLegacyClasses().

This is probably fine BUT it leaves the problem for us to encounter later. Shouldn't we just have separate connections for each SR?

Greg Shah wrote:

Shouldn't we just have separate connections for each SR?

Looking into that.

I see that for the H2 databases accessed in embedded mode, there is the AUTO_SERVER option which allows "multiple processes to access the same database without having to start the server manually." (https://www.h2database.com/html/features.html#auto_mixed_mode). I tried using it but some processes still get that error. Disabling the autocommit or adding a LOCK_TIMEOUT didn't help.

OK, sorry I didn't think carefully enough about this issue. The problem is the multi-process nature of the testing approach. It is not a thread safety issue. Our code would need more changes to allow multiple processes to share the same database instance. One of the processes would have to be a database server and the others would have to connect to that server. We don't need to implement that approach.

Please move ahead with the change to pass the dict=false to the SymbolResolver constructor.

These tests seem reasonable. You can check them in.

I think we do need more complex tests, which show that these things lex properly when there are many tokens. I'm worried that these tests are mostly just about lexing a single token at a time, which is fine for a start but does not fully prove that the lexer is working. These tests are not full 4GL programs but just "fragments".

SO: we can implement these "simple" single-token tests but we really need actual 4GL code (full statements or multiple lines of statements) that includes the tokens we are trying to test.

Thanks for the review.

Greg Shah wrote:

These tests seem reasonable. You can check them in.

I think we do need more complex tests, which show that these things lex properly when there are many tokens. I'm worried that these tests are mostly just about lexing a single token at a time, which is fine for a start but does not fully prove that the lexer is working. These tests are not full 4GL programs but just "fragments".

SO: we can implement these "simple" single-token tests but we really need actual 4GL code (full statements or multiple lines of statements) that includes the tokens we are trying to test.

Assuming the purpose of a testcase is to test a specific Lexer rule path, why would we want to add extra code (which probably would correspond to different lexer/parser rules)? Wouldn't that just make it harder to pin-point the cause of failure in case the harness test fails? Could you maybe give a brief example of how a complete test should look like in your view? Let's take for example the fragment

 .\\filename.p. 

[00001:001] <FILENAME>                            .\\filename.p
[00001:014] <DOT>                                 .
[00001:015] <EOF>                                 null

- we can already draw the conclusion that the lexer is right. For example the token name is <FILENAME> and not <SYMBOL> for example, and the .p is not a separate token, etc.

I'm worried that these tests are mostly just about lexing a single token at a time.

Could you please also clarify what you meant by that? Aren't all the tokens lexed sequentially?

we can already draw the conclusion that the lexer is right

Not fully. It tells us that the lexer rule that matches this specific construct is correct. Without more complex input, it cannot test potential ambiguity that might cause lexing to predict a different rule for matching. There are constructs in the lexer that might match the same text from different rules. We can't only test the "simple" inputs. We must have more complex cases as well.

For some reason, the previous commit did not push the files. I did another commit (see #1889-12) and it succeeded.

Greg, please review the last changes from #6860a and let me know if I can merge it.

Code Review Task Branch 6860a Revisions 15884 through 15886

LexerTestDriver:

• use wildcard imports
• add javadoc for all data members

Please rename LexerTestOptions to LexerOptions and move it back to the uast package so that we don't have references to child packages from parent packages.

Greg Shah wrote:

Code Review Task Branch 6860a Revisions 15884 through 15886

LexerTestDriver:

• use wildcard imports
• add javadoc for all data members

Greg Shah wrote:

Please rename LexerTestOptions to LexerOptions and move it back to the uast package so that we don't have references to child packages from parent packages.

I solved these and committed to 6860a/15887.

Code Review Task Branch 6860a REvision 15887

Please remove import com.goldencode.p2j.uast.LexerOptions; from LexerDumpFilter.

Also, com.goldencode.p2j.uast.LexerOptions needs to be checked in.

Greg Shah wrote:

Code Review Task Branch 6860a REvision 15887

Please remove import com.goldencode.p2j.uast.LexerOptions; from LexerDumpFilter.

Also, com.goldencode.p2j.uast.LexerOptions needs to be checked in.

Done. Committed to 15888.

Greg Shah wrote:

Feedback on the tests:

• The date tests are missing a large number of cases that can be found in date.main(). We need to ensure that all of those various values are tested as date literals (quoted and unquoted).
• We need to work through a similar set of testcases for these literals:
  • num/dec/hex
  • logical
  • unknown value
  • datetime
  • datetime-tz
  • unquoted text

There are some basic expression 'literals' tests in tests/expressions/TestLiterals, quoted values aren't allowed for dates but when used with date built in function then those should be tested already in tests/base_language/builtin_functions/functionality/TestDate (TestInteger, TestInt64, TestDecimal, TestLogical...).

quoted values aren't allowed for dates

There is an exception to this. In OE (and FWD), the INITIAL clause of a DEF VAR statement allows quoted strings that are dates (and logicals...). It is stupid and nasty, but it exists.

Greg Shah wrote:

quoted values aren't allowed for dates

There is an exception to this. In OE (and FWD), the INITIAL clause of a DEF VAR statement allows quoted strings that are dates (and logicals...). It is stupid and nasty, but it exists.

True, although those behave like date builtin function when passing a string into I've extended the test class to cover this initial case.

Greg Shah wrote:

Code Review Task Branch 6860a Revision 15888

The code is good.

The changes are pretty safe. What testing has been done?

Besides making sure it's safe to run many tests in parallel and actually running them with harness test plans, no testing has been done. Should we do ChUI regression tests?

Greg Shah wrote:

• We need to work through a similar set of testcases for these literals:
  • num/dec/hex
  • logical
  • unknown value
  • datetime
  • datetime-tz
  • unquoted text

Do you mean testing these specifically with DEF VAR ... INIT statement, or more general tests? For unqoted text do unquoted date/datetime/datetimetz literals count?

Marian Edu wrote:

There are some basic expression 'literals' tests in tests/expressions/TestLiterals, quoted values aren't allowed for dates but when used with date built in function then those should be tested already in tests/base_language/builtin_functions/functionality/TestDate (TestInteger, TestInt64, TestDecimal, TestLogical...).

Thanks, these really are rich and can be turned into lexer tests.

Besides making sure it's safe to run many tests in parallel and actually running them with harness test plans, no testing has been done. Should we do ChUI regression tests?

I think that makes sense. I don't think we need the runtime tests, just the conversion regression testing.

Also, check Hotel GUI conversion (run conversion in trunk and in 6860a and compare the results).

In both cases, if there are no errors during front end processing and all the conversion results are the same, then we can merge this.

• We need to work through a similar set of testcases for these literals:
  • num/dec/hex
  • logical
  • unknown value
  • datetime
  • datetime-tz
  • unquoted text

Do you mean testing these specifically with DEF VAR ... INIT statement, or more general tests?

Both.

For unqoted text do unquoted date/datetime/datetimetz literals count?

No, I'm referring specifically to anything in the lexer that resolves to the UNQUOTED_TEXT token. This is a weird 4GL feature of FORM statements where there is special processing for sequences of continuous unquoted MINUS or PLUS characters (e.g. ------ or +++).

Branch 6860a passed the regression tests on ChUI and Hotel conversions, but I'm thinking we could delay the merge since Greg mentioned that we need tests for keyword ignore processing. Therefore I should probably add a feature to pass an ignore file to the lexet test driver.

Greg, speaking of keyword ignore processing tests, could you please clarify

1. What did you mean by

• I suspect this area will be tricky because OE may treat these quite differently than random strings and other literals. Where in FWD it will be easy to test the result, in OE we may have to rely upon some stubbed out 4GL code that allows the keywords to be >added.

2. How would you suggest to test the ignore keywords feature? I am thinking of taking a smaller sample of keywords, ignoring one at a time and testing the lexing.

3. How would you suggest testing the case case insensitivity, is duplicating all keywords tests the way to go?

4. For reserved/unreserved keywords, we could define variables and check if they are lexed as <SYMBOL> or <KW_...>, although I am not sure whether we should be doing that for all reserved/unreserved keywords.

• num/dec/hex
• logical
• unknown value
• datetime
• datetime-tz
• unquoted text
• filenames
• keywords - WIP

Branch 6860a passed the regression tests on ChUI and Hotel conversions, but I'm thinking we could delay the merge since Greg mentioned that we need tests for keyword ignore processing. Therefore I should probably add a feature to pass an ignore file to the lexet test driver.

Yes, it makes sense to pass the keyword ignore file in via lexer options. Please note that the lexer itself already provides full keyword ignore support. See keyword-ignore in Configuration Reference.

1. What did you mean by

• I suspect this area will be tricky because OE may treat these quite differently than random strings and other literals. Where in FWD it will be easy to test the result, in OE we may have to rely upon some stubbed out 4GL code that allows the keywords to be >added.

It may be valid to have an entire 4GL program that is just "a string" and the compiler will allow it. The early tests you've written rely upon this idea that you can put some of these contents into a file and it is syntactically OK.

But when you get to testing keywords in OE, I think you can't just put "DEFINE" in a file and try to compile it. Keywords are going to require language syntax or they will fail the OE compile.

2. How would you suggest to test the ignore keywords feature? I am thinking of taking a smaller sample of keywords, ignoring one at a time and testing the lexing.

I would randomly select 10-20 keywords, including a good variety of reserved and unreserved. Then I would test keyword processing with those in the ignore list. Make sure the ignore list itself encodes the keywords in a variety of ways (case insensitively, with and without abbreviations).

You don't have to test all possible keywords, just a wide enough selection to confirm that we match the same behavior as OE for keyword ignore. Note that this feature is rarely used so we don't want to go crazy here.

3. How would you suggest testing the case case insensitivity, is duplicating all keywords tests the way to go?

Yes

4. For reserved/unreserved keywords, we could define variables and check if they are lexed as <SYMBOL> or <KW_...>, although I am not sure whether we should be doing that for all reserved/unreserved keywords.

Yes and Yes

Greg, I looked into testing the reserved/unreserved keyword for FWD by defining variables. I'm thinking of checking the final token types, the ones after the parsing rules make the needed corrections. In case of invalid variable name, such as

 def var true as int. 

the parser fails with an exception, we can use the initial token type to test, or we can just assume that the test passed. (the test would pass in this case, because it means the token wasn't converted into a symbol).

If a variable name is valid, such as def var absolute as int. we would get the full AST, such as

block [BLOCK]:<id_unavailable> @0:0
   statement [STATEMENT]:<id_unavailable> @0:0
      define [DEFINE_VARIABLE]:<id_unavailable> @1:1
         absolute [SYMBOL]:<id_unavailable> @1:17
         as [KW_AS]:<id_unavailable> @1:26
            integer [KW_INT]:<id_unavailable> @1:29

And we can check the final token type (should be SYMBOL).

This would require that we run the parser for lexer tests. We can do that, but seems a bit complicated. Why not just add some logic to check if the token type for the keyword is between BEGIN_RESERVED and END_RESERVED? That means it is a reserved keyword. Anything between BEGIN_UNRESERVED and END_UNRESERVED is an unreserved keyword. This is more direct and easier to understand, I think. Also, it avoids having to rely upon the parser to test the lexer.

Greg Shah wrote:

Why not just add some logic to check if the token type for the keyword is between BEGIN_RESERVED and END_RESERVED?

OK, then I'll do that.

Slight change committed to 6860a/15891:

We shall test the keywords in the following way:

instead of

java com.goldencode.p2j.uast.test.LexerTestDriver -test-keyword DEFINE

the structure will be this

java com.goldencode.p2j.uast.test.LexerTestDriver -test-keyword DEFINE <EXPECTED_CLASS>

where the expected class can be [RESERVED-KEYWORD | UNRESERVED-KEYWORD | NOT-KEYWORD]. This is because harness can't really deal with the java output consistently, so we need an Assert.equals() and test with respect to BUILD FAIL or BUILD SUCCESSFULL.

OK, that is reasonable.

Code Review Task Branch 6860a Revisions 15889 through 15991

Overall, the changes are good.

1. I'm not sure about the value of isKeyword() and isReservedKeyword(). We can't put all such logic inside the lexer. Let's leave it for now. These methods need some cleanup to meet coding standards:

• KW should not be used since this isn't a constant data member of the class.
• Please use parenthesis to group the operators to explicitly show precedence.
• Please fix the indentation.

2. Instead of the "ABL" name (which at one point Progress claimed was a trademark), please use the term 4GL. This should be used for method names, var names and even in comments.

3. In LexerTestDriver, is import antlr.debug.misc.ASTFrame; needed?

4. In LexerTestDriver, please split the { back onto its own line.

Not really related to the task topic though it does affect the `testcases` projects (at lest for those using windows)... @Florin, can you please rename the `tests/conversion/lexer/how to run the tests?` file to something maybe equally verbose but can work both on *nix/windows? (sic, the question mark is a reserved character)

Marian Edu wrote:

Not really related to the task topic though it does affect the `testcases` projects (at lest for those using windows)... @Florin, can you please rename the `tests/conversion/lexer/how to run the tests?` file to something maybe equally verbose but can work both on *nix/windows? (sic, the question mark is a reserved character)

For sure, I was planning to fix that in my next commit, unless it's urgent and you'd like me to do it right away?

Florin Eugen Rotaru wrote:

Marian Edu wrote:

Not really related to the task topic though it does affect the `testcases` projects (at lest for those using windows)... @Florin, can you please rename the `tests/conversion/lexer/how to run the tests?` file to something maybe equally verbose but can work both on *nix/windows? (sic, the question mark is a reserved character)

For sure, I was planning to fix that in my next commit, unless it's urgent and you'd like me to do it right away?

Done.

I have committed the tests for all keywords to the Testcases repository.

• tests/conversion/lexer/4gl_testcases/reserverd contains the tests for checking what keywords are allowed as variables. The tests are only for OE, the FWD ones are solely the harness .xml files which use the driver's -test-keyword feature. The ignore-keywords feature is tested here too. When the tests are run, an ignore file is passed which has aprox. 20 keywords.

• tests/conversion/lexer/4gl_testcases/general_tests contains the tests for all keywords - for each keyword there is small relevant testcase which puts it into a context. These tests include both lowercase tests and uppercase tests.

For some keywords I could not find sufficient info in order to put them in a test:

Key = pixels; Value = 'pixels' minChars = 0; tokenType = 796; reserved = true
Key = _serial-n; Value = '_serial-num' minChars = 7; tokenType = 519; reserved = true
Key = can-load; Value = 'can-load' minChars = 0; tokenType = 1110; reserved = true
Key = text-cursor; Value = 'text-cursor' minChars = 0; tokenType = 908; reserved = true
Key = schema; Value = 'schema' minChars = 0; tokenType = 860; reserved = true
Key = _serial-nu; Value = '_serial-num' minChars = 7; tokenType = 519; reserved = true
Key = after-row-fill; Value = 'after-row-fill' minChars = 0; tokenType = 979; reserved = false
Key = mouse; Value = 'mouse' minChars = 0; tokenType = 738; reserved = true
Key = from-pixels; Value = 'from-pixels' minChars = 6; tokenType = 676; reserved = true
Key = web.output; Value = 'web.output' minChars = 0; tokenType = 2554; reserved = false
Key = from-chars; Value = 'from-chars' minChars = 6; tokenType = 675; reserved = true
Key = precision; Value = 'precision' minChars = 0; tokenType = 2095; reserved = false
Key = get-user-field; Value = 'get-user-field' minChars = 0; tokenType = 1559; reserved = false
Key = outputhttpheader; Value = 'outputhttpheader' minChars = 0; tokenType = 2049; reserved = false
Key = row-update; Value = 'row-update' minChars = 0; tokenType = 2236; reserved = false
Key = default-noxlate; Value = 'default-noxlate' minChars = 12; tokenType = 606; reserved = true
Key = before-fill; Value = 'before-fill' minChars = 0; tokenType = 1041; reserved = false
Key = get-long-value; Value = 'get-long-value' minChars = 0; tokenType = 1520; reserved = false
Key = get-value; Value = 'get-value' minChars = 0; tokenType = 1560; reserved = false
Key = proc-text; Value = 'proc-text' minChars = 0; tokenType = 2120; reserved = false
Key = prodataset; Value = 'prodataset' minChars = 0; tokenType = 2122; reserved = false
Key = define-user-event-manager; Value = 'define-user-event-manager' minChars = 0; tokenType = 1275; reserved = false
Key = process-web-request; Value = 'process-web-request' minChars = 0; tokenType = 2119; reserved = false
Key = analyze; Value = 'analyze' minChars = 6; tokenType = 530; reserved = true
Key = queue-message; Value = 'queue-message' minChars = 0; tokenType = 2148; reserved = false
Key = output-content-type; Value = 'output-content-type' minChars = 0; tokenType = 2047; reserved = false
Key = hidden-field-list; Value = 'hidden-field-list' minChars = 0; tokenType = 1594; reserved = false
Key = set-cookie; Value = 'set-cookie' minChars = 0; tokenType = 2326; reserved = false
Key = output-messages; Value = 'output-messages' minChars = 0; tokenType = 2050; reserved = false
Key = get-cookie; Value = 'get-cookie' minChars = 0; tokenType = 1579; reserved = false
Key = setcookie; Value = 'setcookie' minChars = 0; tokenType = 2326; reserved = false
Key = log-audit-event; Value = 'log-audit-event' minChars = 0; tokenType = 1762; reserved = false
Key = can-dump; Value = 'can-dump' minChars = 0; tokenType = 1109; reserved = true
Key = column-sorting; Value = 'column-sorting' minChars = 0; tokenType = 2790; reserved = false
Key = before-row-fill; Value = 'before-row-fill' minChars = 0; tokenType = 1042; reserved = false
Key = last-key; Value = 'last-key' minChars = 0; tokenType = 723; reserved = true
Key = closeallprocs; Value = 'closeallprocs' minChars = 0; tokenType = 1151; reserved = false

Also, for some keywords I keep seeing // not a real keyword, but is normally defined in the PSC webspeed 4GL code. I couldn't find tests for them as well.

Marian Edu wrote:

Not really related to the task topic though it does affect the `testcases` projects (at lest for those using windows)... @Florin, can you please rename the `tests/conversion/lexer/how to run the tests?` file to something maybe equally verbose but can work both on *nix/windows? (sic, the question mark is a reserved character)

Marian
In testcases rev 1756 you have added <import file="build_ablunit.xml" /> to build.xml. Note that breaks building testcases under Linux for FWD.
Is there a reason for that ? Before build_ablunit.xml was a separate build file to be executed under Windows/OE only.
I guess that should be conditional and imported only when using OE, shoulnd't it ?

Greg Shah wrote:

Code Review Task Branch 6860a Revisions 15889 through 15991

Overall, the changes are good.

1. I'm not sure about the value of isKeyword() and isReservedKeyword(). We can't put all such logic inside the lexer. Let's leave it for now. These methods need some cleanup to meet coding standards:

• KW should not be used since this isn't a constant data member of the class.
• Please use parenthesis to group the operators to explicitly show precedence.
• Please fix the indentation.

2. Instead of the "ABL" name (which at one point Progress claimed was a trademark), please use the term 4GL. This should be used for method names, var names and even in comments.

3. In LexerTestDriver, is import antlr.debug.misc.ASTFrame; needed?

4. In LexerTestDriver, please split the { back onto its own line.

I have refactored the code and committed to 6860a/15892

I made a new commit to testcases. I've added tests and harness tests for the missing branches of the lexer rules according to the code coverage reports.

There are still some missing cases which are not covered. There are:

1. The default: clauses added automatically by ANTLR for the case statements.
2. Some if's like if ( _createToken && _token==null && _ttype!=Token.SKIP ) also added by ANTLR.
3. Some string literals such as '\r' or '\t' which can't be matched because they've been cleared away by the preprocessor.

The coverage report is in testcases/tests/conversion/lexer/coverage.

Overall, the changes are very good. Have you made any changes to fill in code coverage gaps?

It is a little confusing that the unreserved keywords are tested in code from tests/conversion/lexer/4gl_testcases/keywords/reserved/valid/.

I've added tests and harness tests for the missing branches of the lexer rules according to the code coverage reports.

Based on the reports it seems there are still some gaps, but I think we've spent enough time on this for now.

I've rebased branch 6860a to rev. 16007. I have tested Hotel GUI conversion and also ChUI regression (conversion only) and haven't noticed any changes.

Can we proceed with the merge?

Branch 6860a was merged into trunk as rev. 15992 and archived.

We need to write documentation for this test suite in Lexer Testcases.