Feature ASH language server features

fronobulax

fronobulax

Developer
Staff member
I did ant daily (to update and build 20838) and then ant test. The latter failed.

Code: 
    [junit] Testcase: testScriptValidity[Script with bom](net.sourceforge.kolmafia.textui.ParserTest):  Caused an ERROR
    [junit] Script parsing error ()
    [junit] net.sourceforge.kolmafia.textui.ScriptException: Script parsing error ()
    [junit]     at net.sourceforge.kolmafia.textui.Parser.parseException(Parser.java:4698)
    [junit]     at net.sourceforge.kolmafia.textui.Parser.parse(Parser.java:224)
    [junit]     at net.sourceforge.kolmafia.textui.ParserTest.testScriptValidity(Unknown Source)
    [junit]     at jdk.internal.reflect.GeneratedMethodAccessor1.invoke(Unknown Source)
    [junit]     at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)

I'm hoping it is a trivial result of checking in and the running tests and that it is related to this work.
 
F

fredg1

Member
Hard to test, Clover doesn't even let me run them...
Code: 
Buildfile: C:\Users\Frederic\Desktop\KoLmafia-workspace\kolmafia\build.xml

init:

svn-version:

gitsvn-version:

version:
     [echo] Current revision: 20838M

compile-deps:

compile:

svn-version:

gitsvn-version:

version:
     [echo] Current revision: 20838M

set.version:
    [javac] Compiling 1 source file to C:\Users\Frederic\Desktop\KoLmafia-workspace\kolmafia\build
    [javac] Ignoring source, target and bootclasspath as release has been set

svn-version:

gitsvn-version:

version:
     [echo] Current revision: 20838M

unset.properties:

test-compile:
    [javac] Compiling 1 source file to C:\Users\Frederic\Desktop\KoLmafia-workspace\kolmafia\build
    [javac] warning: [options] bootstrap class path not set in conjunction with -source 8
    [javac] 1 warning

test:
    [junit] Clover estimates having saved around 6 hours on this optimized test run. The full test run takes approx. 6 hours
    [junit] Clover included 5 test classes in this run (total # test classes : 17)
    [junit] Running net.sourceforge.kolmafia.request.ChatRequestTest
    [junit] Testsuite: net.sourceforge.kolmafia.request.ChatRequestTest
    [junit] Tests run: 5, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 2,482 sec
    [junit] Tests run: 5, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 2,482 sec
    [junit]
    [junit] Running net.sourceforge.kolmafia.utilities.IntWrapperTest
    [junit] Testsuite: net.sourceforge.kolmafia.utilities.IntWrapperTest
    [junit] Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0,052 sec
    [junit] Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0,052 sec
    [junit]
    [junit] Running net.sourceforge.kolmafia.utilities.KoLDatabaseTest
    [junit] Testsuite: net.sourceforge.kolmafia.utilities.KoLDatabaseTest
    [junit] Tests run: 4, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0,031 sec
    [junit] Tests run: 4, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0,031 sec
    [junit]
    [junit] Running net.sourceforge.kolmafia.utilities.LowerCaseEntryTest
    [junit] Testsuite: net.sourceforge.kolmafia.utilities.LowerCaseEntryTest
    [junit] Tests run: 3, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 2,229 sec
    [junit] Tests run: 3, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 2,229 sec
    [junit]
    [junit] Running net.sourceforge.kolmafia.utilities.RollingLinkedListTest
    [junit] Testsuite: net.sourceforge.kolmafia.utilities.RollingLinkedListTest
    [junit] Tests run: 5, Failures: 0, Errors: 0, Skipped: 1, Time elapsed: 0,104 sec
    [junit] Tests run: 5, Failures: 0, Errors: 0, Skipped: 1, Time elapsed: 0,104 sec
    [junit]
    [junit] Testcase: itShouldBehaveForABulkAdd(net.sourceforge.kolmafia.utilities.RollingLinkedListTest):SKIPPED: Class under test needs to be augmented

BUILD SUCCESSFUL
Total time: 14 seconds
Press any key to continue . . .

Terminal will be reused by tasks, press any key to close it.

@fronobulax Could you apply this patch and restart the tests? It's the error message you got, modified to give more information (one of the things I planned to add later, anyway).
It should help us find what's happening.
 

Attachments

  • Hindsightful_error_message.patch
    710 bytes · Views: 1
H

heeheehee

Developer
Staff member
So, you may have noticed that in one of the changes (I think the big one, 20835) I made a change to the parameterized test name. OpenClover doesn't want to read it without extra effort, but it was just providing an index previously...

Anyways, it looks like the test case that's failing for frono is "Script with bom", which looks like
Code: 
\ufeff    'hello world'

Although, something I'm observing:

Code: 
            if ( line.length() > 0 &&
                 line.charAt( 0 ) == Parser.BOM )
Should this be line.codePointAt, seeing as the BOM is a unicode codepoint that's actually 2 bytes?

@fronobulax , can you see if the test passes with that change?

edit: we'd presumably also need to change BOM to be an int or other type that can actually hold 2 bytes.
 
fronobulax

fronobulax

Developer
Staff member
Code: 
    [junit] Testcase: testScriptValidity[Script with bom](net.sourceforge.kolmafia.textui.ParserTest):  Caused an ERROR
    [junit] Script parsing error: we didn't reach the end of the file.
    [junit] Current line reads ?    'hello world'
    [junit] Next line is end of file. ()
    [junit] net.sourceforge.kolmafia.textui.ScriptException: Script parsing error: we didn't reach the end of the file.
    [junit] Current line reads ?    'hello world'
    [junit] Next line is end of file. ()
    [junit]     at net.sourceforge.kolmafia.textui.Parser.parseException(Parser.java:4703)
    [junit]     at net.sourceforge.kolmafia.textui.Parser.parse(Parser.java:224)
    [junit]     at net.sourceforge.kolmafia.textui.ParserTest.testScriptValidity(Unknown Source)
    [junit]     at jdk.internal.reflect.GeneratedMethodAccessor1.invoke(Unknown Source)
    [junit]     at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)

Patch applied and run.

At least 12 hours before I will get back to this but will try and help.
 
F

fredg1

Member
While this gets sorted out, here are a variety of small patches:
  1. Parser's public static fields "probably" ought to be final
  2. Removal of redundant method parsePostCall.
    • Said method was called at the very end of parseCall and parseInvoke.
    • Both parseCall and parseInvoke are (*only*) called by parseValue.
    • parseValue ends with [[[paused to answer to frono's post]]]
      *ahem*...
    • parseValue ends with the same logic as parsePostCall, in addition to making it act on an additional case.
    • This effectively makes parsePostCall redundant, as anything it does would have been done anyway in parseValue
Code_YliUYNpwKu.png
 

Attachments

  • final_static_fields.patch
    837 bytes · Views: 1
  • parsePostCall_vs_parseValue.patch
    1.5 KB · Views: 1
Last edited:
F

fredg1

Member
fronobulax said:
Code: 
    [junit] Testcase: testScriptValidity[Script with bom](net.sourceforge.kolmafia.textui.ParserTest):  Caused an ERROR
    [junit] Script parsing error: we didn't reach the end of the file.
    [junit] Current line reads ?    'hello world'
    [junit] Next line is end of file. ()
    [junit] net.sourceforge.kolmafia.textui.ScriptException: Script parsing error: we didn't reach the end of the file.
    [junit] Current line reads ?    'hello world'
    [junit] Next line is end of file. ()
    [junit]     at net.sourceforge.kolmafia.textui.Parser.parseException(Parser.java:4703)
    [junit]     at net.sourceforge.kolmafia.textui.Parser.parse(Parser.java:224)
    [junit]     at net.sourceforge.kolmafia.textui.ParserTest.testScriptValidity(Unknown Source)
    [junit]     at jdk.internal.reflect.GeneratedMethodAccessor1.invoke(Unknown Source)
    [junit]     at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
Click to expand...
So, what I think I'm seeing here, is that the Byte Order Mark was somehow not skipped...
 
F

fredg1

Member
More accurately, it seems like the Byte Order Mark was not *recognized*...
Which leads to it not being trimmed, which leads to none of the parsing methods finding anything.
parseScope then assumed that "finding nothing means we're at the end of the file".
 
H

heeheehee

Developer
Staff member
This seems non-portable what with endianness unless we do something like
Code: 
    private static final String BOM = "\ufeff";
...
            if ( line.length() > 0 &&
                 line.startsWith( Parser.BOM ) )
            {
                line = line.substring( Parser.BOM.length() );
                offset += Parser.BOM.length();
            }

There's a secondary issue, namely that substring(1) is the wrong thing to do here, when our BOM is actually 2 characters wide.
 
fronobulax

fronobulax

Developer
Staff member
heeheehee said:
So, you may have noticed that in one of the changes (I think the big one, 20835) I made a change to the parameterized test name. OpenClover doesn't want to read it without extra effort, but it was just providing an index previously...

Anyways, it looks like the test case that's failing for frono is "Script with bom", which looks like
Code: 
\ufeff    'hello world'

Although, something I'm observing:

Code: 
            if ( line.length() > 0 &&
                 line.charAt( 0 ) == Parser.BOM )
Should this be line.codePointAt, seeing as the BOM is a unicode codepoint that's actually 2 bytes?

@fronobulax , can you see if the test passes with that change?

edit: we'd presumably also need to change BOM to be an int or other type that can actually hold 2 bytes.
Click to expand...


I made the codePointAt change in Parser and the error didn't change. Sorry. I am going to revert the patch and my change and see what happens tomorrow.
 
F

fredg1

Member
Code_WrVHS5DBj4.pngIs this platform-dependant or something? Because I'm not getting this (also how would it have passed the test for us if that was the case?)
 
H

heeheehee

Developer
Staff member
I... I don't know. It shouldn't be? Maybe a Java version / distribution? or something OS-dependent?

I spent some time staring at the Java docs re: char / String / substring / length. The BOM is part of the "Basic Multilingual Plane" (so, fits in a single char), and all of these methods are defined in terms of chars (which are evidently 2 bytes in Java... too much C/C++ for me...). I'm out of ideas.
 
H

heeheehee

Developer
Staff member
An observation: in the build.xml, the javac task in compile / compile-deps sets encoding="utf-8" but not in test-compile or coverage-compile.

I've committed r20839 in the hopes that it makes a difference. I'm skeptical, though.
 
fronobulax

fronobulax

Developer
Staff member
Reverted local changes and updated to 20839. Same test failure.

I (re)applied the test patch with more informative logging.

Code: 
    [junit] Testcase: testScriptValidity[Script with bom](net.sourceforge.kolmafia.textui.ParserTest):  Caused an ERROR
    [junit] Script parsing error: we didn't reach the end of the file.
    [junit] Current line reads ?    'hello world'
    [junit] Next line is end of file. ()
    [junit] net.sourceforge.kolmafia.textui.ScriptException: Script parsing error: we didn't reach the end of the file.
    [junit] Current line reads ?    'hello world'
    [junit] Next line is end of file. ()
    [junit]     at net.sourceforge.kolmafia.textui.Parser.parseException(Parser.java:4703)
    [junit]     at net.sourceforge.kolmafia.textui.Parser.parse(Parser.java:224)
    [junit]     at net.sourceforge.kolmafia.textui.ParserTest.testScriptValidity(Unknown Source)
    [junit]     at jdk.internal.reflect.GeneratedMethodAccessor1.invoke(Unknown Source)
    [junit]     at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)

Since the question was raised, I'm on Windows 10 and
Code: 
$ java -version
java version "13.0.2" 2020-01-14
Java(TM) SE Runtime Environment (build 13.0.2+8)
Java HotSpot(TM) 64-Bit Server VM (build 13.0.2+8, mixed mode, sharing)
 
H

heeheehee

Developer
Staff member
I've tried:

java-16-openjdk
java-16-jdk (Oracle Java)
java-11-openjdk
java-8-openjdk

all on Linux, without any test failures. My guess is that it's something about Windows, but I'll need to spend some time later to figure that out.
 
fronobulax

fronobulax

Developer
Staff member
Well I am using Oracle 13 :cool:

I recall tracking down and fixing a bug in our product that ultimately was due to what Windows considered the default character set and what other character sets were available. That was long ago but maybe if I actually try and look at the code under test my memory will be jogged.
 
fronobulax

fronobulax

Developer
Staff member
Well...

I got PO'd at my IDE (because I couldn't configure it to run KoLmafia in the debugger. My fault because of how I migrated to a new machine). I got that fixed and ran ParserTest in the debugger. No test error! But ant test at both a Cygwin command line (my usual) and a Windows command prompt both failed. My eyes are glazing over so break time.
 
fronobulax

fronobulax

Developer
Staff member
Hypothesis - the IDE swallows exceptions before they get to the test code. Therefore the test will pass in the IDE but fail at the command line.

Circa line 222 in Parser the debugger thinks the currentToken is ";". This confuses me since the string being passed to the parser does not contain the ; character and so something programmatically may have added it as a token.

At the same time and place the currentLine is reported as "null", specifically the string "null" and not the expected null reference. That could be correct but it seems counter intuitive. I believe String.valueOf(null) returns "null" so perhaps something that needs to check for null is not doing so;

Break time.
 
F

fredg1

Member
fronobulax said:
Hypothesis - the IDE swallows exceptions before they get to the test code. Therefore the test will pass in the IDE but fail at the command line.

Circa line 222 in Parser the debugger thinks the currentToken is ";". This confuses me since the string being passed to the parser does not contain the ; character and so something programmatically may have added it as a token.

At the same time and place the currentLine is reported as "null", specifically the string "null" and not the expected null reference. That could be correct but it seems counter intuitive. I believe String.valueOf(null) returns "null" so perhaps something that needs to check for null is not doing so;

Break time.
Click to expand...
The "end of file" Line (or what we marked as such) contains a single Token, with a value of ";"
You can see it being assigned on line 4571
 
F

fredg1

Member
@fronobulax could you try with this? I significantly changed the error message, again, to print a variety of information about Byte Order Marks and the Line's first character.
 

Attachments

  • Hindsightful_error_message_v2.patch
    2.9 KB · Views: 1
fronobulax

fronobulax

Developer
Staff member
fredg1 said:
The "end of file" Line (or what we marked as such) contains a single Token, with a value of ";"
You can see it being assigned on line 4571
Click to expand...

OK. In my days of parsing we didn't add anything to what was being parsed and so I wasn't sure it was deliberate.

I'm still poking at curretLine being "null" in the debugger and still wish I understood while a test that throws an (unexpected) Exception doesn't fail in the IDE.
 
You must log in or register to reply here.
Top