Changes for https://github.com/antlr/antlr4/issues/2693#2936
Conversation
|
Hi |
Eric, I can't write a unit test for this because there is not a single unit test in the C# runtime that I can find, not a single .cs file with an attributed method for testing, not a single *.csproj that references any of the major testing packages (MSTest, NUnit, XUnit, ...). In other words, Normally, in Appveyor, one adds a line to appveyor.yml something like what I do for Antlrvsix here, then write C# tests, which should have been ported from the unit test suite for the Java runtime in the first place--if there is one. Where are the unit tests, written in C# for the C# runtime? Please give me a single file with C# code with a unit test for the C# runtime. Give me a line number with an attributed unit test method in C#. But, there are a number of things with this build I don't understand:
--Ken |
|
Hi Ken,
thanks for your reply.
The testing is done from the java project, and comprises for each test: generation of the lexer, parser, listener etc… build them and run them.
That is why you are not seeing any standard .Net test.
The value of the above approach is that it guarantees that the exact same grammar produces the exact same behavior regardless of the target.
It does make testing of a given target a bit more difficult…
Eric
… Le 12 oct. 2020 à 13:35, Ken Domino ***@***.***> a écrit :
Hi
thanks for this.
I don't have a problem with this, but how do we know it works (better than the current version)?
Would you be able to provide a test?
Eric,
I can't write a unit test for this because there is not a single unit test in the C# runtime that I can find, not a single .cs file with an attributed method for testing, not a single *.csproj that references any of the major testing packages (MSTest, NUnit, XUnit, ...).
Normally, in Appveyor, one adds a line to appveyor.yml something like what I do for Antlrvsix here <https://github.com/kaby76/AntlrVSIX/blob/master/appveyor.yml#L19>, then write C# tests, which should have been ported from the unit test suite for the Java runtime in the first place--if there is one.
Where are the unit tests, written in C# for the C# runtime? Please give me a single file with C# code with a unit test for the C# runtime. Give me a line number with an attributed unit test method in C#.
But, there are a number of things with this build I don't understand:
Take any .sln file in runtime-testsuite\target\classes\CSharp\runtime\CSharp and open it. Try to do a build. No exe and no dll are produced. It just references builds the Antlr library. No tests with C# code are generated, or compiled.
runtime-testsuite\test\org\antlr\v4\test\runtime\csharp\Antlr4.Test.vs2013.csproj makes reference to "L.cs" and "Test.cs". These files don't exist anywhere, and are not generated, or compiled, and it is not listed in the verbose build output.
Why does the build use the ancient toolchain VS2017 (https://github.com/antlr/antlr4/blob/master/appveyor.yml#L5 <https://github.com/antlr/antlr4/blob/master/appveyor.yml#L5>)? Just use NET Core or VS2019.
Why is the VS2013 csproj (non-SDK style csproj) used (https://github.com/antlr/antlr4/blob/master/appveyor.yml#L15 <https://github.com/antlr/antlr4/blob/master/appveyor.yml#L15>) for testing--which contains zero unit tests, no references nor a testing package? Just use NET Core and the one "dotnet.csproj" file.
Why is the old non-SDK style csproj for Mono provided?
Why, in the build documentation (https://github.com/antlr/antlr4/blob/master/doc/building-antlr.md <https://github.com/antlr/antlr4/blob/master/doc/building-antlr.md>), and in the appveyor build (https://github.com/antlr/antlr4/blob/master/appveyor.yml#L13 <https://github.com/antlr/antlr4/blob/master/appveyor.yml#L13>) does it say to use -DskipTests in the mvn command? Really, "skip tests"???
--Ken
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub <#2936 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAZNQJA2Z2HUR3XAAJ4KSNLSKKIRHANCNFSM4SMBNPQQ>.
|
|
OK, I see after typing in the build commands manually ("mvn install")" MSBuild version 12??! I'm not even sure I want to download and install Msbuild12 toolchain which came out with VS2013! And "mvn clean" doesn't remove these test directories?? I've got hundreds of these directories on my machine. I now see the C# code in the AppData/Local/Temp/BaseCSharpTest* directories. Those aren't exactly "unit tests" of a particular method in the runtime, are they? These would be called "integration tests". How do you propose I write a unit test for the constructor of AmbiguityInfo that I changed? I also now see the closest test in the tool-testsuite, e.g., here and other places. That tests the Java runtime, but it's place under the tool tests, so profiling is never tested on any of the runtimes other than Java. How do you propose I write an integration test for C#? There's no String Template conversion here to generate C# code. |
|
Honestly, I don't know where to go with this. And the bigger problem is that the C# runtime should have been synchronized with the Java runtime 6 years ago. That's what the changes are--just resynching the sources, partially at that. In fact, the Java runtime is being modified without propagating the changes to the other runtimes, e.g., C#. Someone will then need to do a character-by-character diff and verify the runtimes are synchronized, really error-prone and unsafe, and changes are still being accepted for one runtime and not propagated to the other runtimes. This is bad. |
|
Hi
I understand all this can be quite confusing
A few comments :
Testing performance and testing the profiling are 2 different things.
Also the tests are generated from Java, so no need for C# StringTemplate.
And no need to generate anything I think.
My approach would be to leverage an existing test and invoke it in profiling mode after the regular mode. This can be done by enhancing BaseCSharpTest.
This will prove that the profiler runs ok, which is all I’m hoping.
Envoyé de mon iPhone
… Le 12 oct. 2020 à 21:54, Ken Domino ***@***.***> a écrit :
OK, I see after typing in the build commands manually ("mvn install")"
-------------------------------------------------------
T E S T S
-------------------------------------------------------
Running org.antlr.v4.test.runtime.csharp.TestCompositeLexers
.java.io.IOException: Cannot run program ""C:\Program Files (x86)\MSBuild\12.0\Bin\MSBuild.exe"" (in directory
"C:\Users\kenne\AppData\Local\Temp\BaseCSharpTest-main-1602504463472"): CreateProcess error=2, The system
cannot find the file specified
at java.base/java.lang.ProcessBuilder.start(ProcessBuilder.java:1128)
at java.base/java.lang.ProcessBuilder.start(ProcessBuilder.java:1071)
at org.antlr.v4.test.runtime.csharp.BaseCSharpTest.buildProject(BaseCSharpTest.java:389)
MSBuild version 12??! I'm not even sure I want to download and install Msbuild12 toolchain which came out with VS2013! And "mvn clean" doesn't remove these test directories?? I've got hundreds of these directories on my machine.
I now see the C# code in the AppData/Local/Temp/BaseCSharpTest* directories. Those aren't exactly "unit tests" of a particular method in the runtime, are they? These would be called "integration tests".
How do you propose I write a unit test for the constructor of AmbiguityInfo that I changed?
I also now see the closest test in the tool-testsuite, e.g., here and other places. That tests the Java runtime, but it's place under the tool tests, so profiling is never tested on any of the runtimes other than Java. How do you propose I write an integration test for C#? There's no String Template conversion here to generate C# code.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
|
Hi,
I don’t think it’s as bad as you feel.
The only compatibility we really care about is being able to use the same grammar across targets.
There are 10 targets, so no way a single person will update those 10 targets, we just have to live with that.
The profiler is a tool, it can work differently from one target to another without breaking core compatibilit.
So I don’t think there is a need to synchronize the profiler, only to have it working.
Eric
… Le 12 oct. 2020 à 22:21, Ken Domino ***@***.***> a écrit :
Honestly, I don't know where to go with this. And the bigger problem is that the C# runtime should have been synchronized with the Java runtime 6 years ago. That's what the changes are--just resynching the sources, partially at that. In fact, the Java runtime is being modified <f88f763#diff-1e1aff06967135502fb84c3b236836afL81> without propagating the changes to the other runtimes, e.g., C# <https://github.com/antlr/antlr4/blob/master/runtime/CSharp/runtime/CSharp/Antlr4.Runtime/Atn/LexerATNSimulator.cs#L48>. Someone will then need to do a character-by-character diff and verify the runtimes are synchronized, really error-prone and unsafe, and changes are still being accepted for one runtime and not propagated to the other runtimes. This is bad.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub <#2936 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAZNQJA23PUEF7NIKVRXWFLSKMGGRANCNFSM4SMBNPQQ>.
|
|
This is going to take a week or two to finish. I have to fish out the prereq's for doing a build on Windows (though it is tested with Appveyor on Windows) and cram this into BaseCSharpTest.java or nearby. |
|
another approach would be to have a pre-generated parser and lexer, and a .Net test project that links to the just built dll
the test project would be about tooling (as opposed to parsing)
then run the tests as a cmd line in the CI yaml and check for failures
no java required, and I’m fine if it only runs on AppVeyor
… Le 14 oct. 2020 à 15:34, Ken Domino ***@***.***> a écrit :
This is going to take a week or two to finish. I have to fish out the prereq's for doing a build on Windows (though it is tested with Appveyor on Windows) and cram this into BaseCSharpTest.java <https://github.com/antlr/antlr4/blob/master/runtime-testsuite/test/org/antlr/v4/test/runtime/csharp/BaseCSharpTest.java> or nearby.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub <#2936 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAZNQJGKXPHDI5DDUBVHHZLSKVIAVANCNFSM4SMBNPQQ>.
|
|
@kaby76 any progress? |
Not yet. I've been busy with Antlrvsix/Trash (fast serialization of Antlr data structures, grammar diffing algorithms), Dart. I'll work on this soon, now post Antlr 4.9. --Ken |
This is a fix for #2693. The change basically rolls over code from the Java runtime to the C# runtime that was never done. With the change, profiling now works. I posted this change a year ago, but because it was ignored until a few days ago, I closed that PR and re-did the PR with the changes here. I don't know what is going on with the Appveyor build of PHP, but the Git check-in is warning me about uncommited changes in ../runtime/PHP, which is a git submodule. I'm not changing anything in PHP, and this PR doesn't include changes to runtime/PHP. The build instructions say nothing about pulling the PHP submodule. The copyright dates are changed to 2019 (from my year-old PR), but every file should be updated Jan 1 with the new year via a script.