Skipped: 3, thats when a unit test is deactivated with the @Ignore annotation, e.g. here github.com/vassalengine/vassal/ … sTest.java
Btw this whole test class is wrong in three ways,
1st you don’t test code that you have not written like library code, and this only really tests ManagementFactory.getOperatingSystemMXBean().getTotalPhysicalMemorySize(),
2nd if it’s supposed to test the Vassal class MemoryUtils then it should mock ManagementFactory.getOperatingSystemMXBean().getTotalPhysicalMemorySize(),
3rd the two @Ignore’d unit tests, why are they ignored, who and when is going to activate them? why are they still there and not removed entirely?
Thus spake Flint1b:
[This message has been edited.]
Skipped: 3, thats when a unit test is deactivated with the @Ignore
annotation, e.g. here github.com/vassalengine/vassal/ …
sTest.java[1]
Btw this whole test class is wrong in three ways,
1st you don’t test code that you have not written like library code, and
this only really tests
ManagementFactory.getOperatingSystemMXBean().getTotalPhysicalMemorySize(),
It’s there for a reason. See the commit message for 1dd402d16.
2nd if it’s supposed to test the Vassal class MemoryUtils then it should
mock
ManagementFactory.getOperatingSystemMXBean().getTotalPhysicalMemorySize(),
The point of the test is not to test MemoryUtils, but to check that we’re
getting a sane answer.
3rd the two @Ignore’d unit tests, why are they ignored, who and when is
going to activate them? why are they still there and not removed
entirely?
The two FIXMEs say why those two tests are ignored. At the time, I didn’t
have a handy way to do the check that’s done on Linux on other OSes. (The
Mac check would probbaly be similar to the Linux one; no idea on Windows.)
I probably figured I’d return to them eventually. You could fill in those
tests. Be the change you want to see in the world.
–
J.
Thus spake Flint1b:
Here. Fixed.
Code:
— a/src/main/java/VASSAL/tools/io/Tailer.java
+++ b/src/main/java/VASSAL/tools/io/Tailer.java
@@ -198,7 +198,7 @@ public class Tailer {
String line;
while ((line = raf.readLine()) != null) {
// readLine strips newlines, we put them back
Ha.
Tailer is working as designed—it’s the test which isn’t loading the
expected data properly.
Proof: Check byte 48 in the output Brian posted. The expected value
printed there is ASCII 13, which is CR. But that’s incorrect. There are no
CRs in src/test/resources/TailerTest.txt. The expected value is supposed
to be ASCII 10, which is what the Tailer reported as the actual value.
I’ve pushed a commit which ensures that the expected results are the
expected results we expect.
–
J.
Only change that would make sense here is to mock the MXBean stuff and ensure that MemoryUtils properly calls the MXBean and returns the values given by MXBean. That is the only job of MemoryUtils. It’s job is not to determine how much memory the computer has, that is the job of the MXBean.
And testing the MXBean is testing library code, not cool, not good, not a good test. How good libraries work is none of our business, we develop Vassal and test Vassal code. Whoever wants to know how libraries work can make a separate project and try it out. This doesn’t belong into the Vassal codebase.
- Yes there are, on Windows machines that have git.autocrlf set to true. Git inserts them.
Lastest commit of PR61 still fails for me.
With the line ending issue, another interesting experiment would be to find out what kind of line endings the actual logfile has on Windows, LF or CRLF. If it’s CRLF then the unit test and its input file represent the given situation and should not be changed.
Thus spake Flint1b:
Only change that would make sense here is to mock the MXBean stuff and
ensure that MemoryUtils properly calls the MXBean and returns the values
given by MXBean. That is the only job of MemoryUtils. It’s job is not
to determine how much memory the computer has, that is the job of the
MXBean.
I’m not testing that MemoryUtils.getPhysicalMemory() returns what
OperatingSystemMXBean gives it. It’s easy to see that MemoryUtils will do
that. I’m testing that MemoryUtils returns a value that’s sane.
And testing the MXBean is testing library code, not cool, not good, not
a good test. How good libraries work is none of our business, we develop
Vassal and test Vassal code. Whoever wants to know how libraries work
can make a separate project and try it out. This doesn’t belong into the
Vassal codebase.
In normal circumstances, one doesn’t test dependencies because in normal
circumstances, you can rely on them to work correctly. When your
dependencies give you reason to doubt that they are working correctly,
however, you are no longer in normal circumstances and it is prudent to
test that your dependencies are in fact producing reasonable results.
This particular class has a history of interacting with unreliable
dependencies. OperatingSystemMXBean is only the most recent of them.
If it starts returning nonsense after a Java version upgrade, I would
very much like to know that as soon as it happens.
There are no
CRs in src/test/resources/TailerTest.txt
- Yes there are, on Windows machines that have git.autocrlf set to
true. Git inserts them.
More accurately: There are no CRs in src/test/resources/TailerTest.txt as
committed, which is what matters.
–
J.
Thus spake Cattlesquat:
Lastest commit of PR61 still fails for me.
I know what I did wrong. I need to do the conversion before taking the
substring, as the conversion changes the length. Try now.
–
J.
Thus spake Flint1b:
With the line ending issue, another interesting experiment would be to
find out what kind of line endings the actual logfile has on Windows, LF
or CRLF. If it’s CRLF then the unit test and its input file represent
the given situation and should not be changed.
The test is for the Tailer’s output. That output goes to the log window.
It doesn’t matater if input to the Tailer has LF or CRLF because the
Tailer used RandomAccessFile.readline() to read that input, and that strips
both kinds of newline.
The failing test demonstrated this very thing: The input file had CRLF
because Git changed the newlines on checkout, but the Tailer produced
only LF.
–
J.
SUCCESS! Merge that BadBoy immediately! 
It’s a good thing, too, because my ability to type “git fetch upstream pull/61/head:Try61” was getting seriously impacted by alcohol at this point on a Covid July 4…
Also PR60 is ready to be merged I think
Ok it works for now but this overcomplicates this otherwise simple matter, it’s much easier to just say, Tailer’s output is supposed to be same to its input, regardless of whether there’s LFs or CRLFs or whatever other newline characters a system might have. Then just let git and Java’s System.lineSeparator() handle the rest.
IBM’s EBCDIC uses hex 15 as line ending. Who knows what Apple will use on it’s new architecture? Are we going to add extra code to TailerTest if they decide that hex 47 is their line ending?
So it’s a test of OperatingSystemMXBean, not of MemoryUtils, and should be renamed to OperatingSystemMXBeanTest. In a proper, clean OOP design, MemoryUtils has a single job, call the MXBean and return the results that MXBean spit out. This and only this belongs in a unit test for MemoryUtils.
I know, I am nuts. Trying to argue with C++ devs about clean OOP, with cohesion, single responsibility principle etc. Go on and keep writing those god classes that are responsible for everything in this macros-around-C joke of a programming language 
Thus spake Flint1b:
IBM’s EBCDIC uses hex 15 as line ending. Who knows what Apple will use
on it’s new architecture? Are we going to add extra code to TailerTest
if they decide that hex 47 is their line ending?
If they use G as their line ending, we can safely ignore them as a relevant
platform from that point onward.
–
J.
Thus spake Flint1b:
So it’s a test of OperatingSystemMXBean, not of MemoryUtils, and should
be renamed to OperatingSystemMXBeanTest. In a proper, clean OOP design,
MemoryUtils has a single job, call the MXBean and return the results
that MXBean spit out. This and only this belongs in a unit test for
MemoryUtils.
There was once quite a lot more to test inside MemoryUtils. It didn’t
start life containing only a simple use of OperatingSystemMXBean, and
when it became that, we had no reason to think it would remain that
way given its history.
By now, it seems safer to say that it won’t change again. If it bothers
you, rename the test.
I know, I am nuts. Trying to argue with C++ devs about clean OOP, with
cohesion, single responsibility principle etc. Go on and keep writing
those god classes that are responsible for everything in this
macros-around-C joke of a programming language
I don’t know who you’re talking to here. I never wrote the classes you’re
referring to start with. I’ve been trying to get rid of them for years.
–
J.
It’s G in the ascii/utf codepage, theoretically they could come up with a new codepage where G is elsewhere, LF is hex 47, CR is 99, tab is 55, and line endings are \r\n\t in a LSB-first 4-byte long int 0x00554799. Just cause it performs faster on their oh-so-cool new 71-bit CPUs
Thus spake Flint1b:
What was the use case again for having several Player instances run at
the same time, that can’t be solved by the user starting several Vassal
instances at the same time?
If the Module Manager starts the Player in the same process, there’s no
way to appropriately adjust the max heap for the module at that point.
–
J.
Yes I get that, what I meant was, why is it allowed to start more than one Player instance from one ModuleManager, what is the use case for this, and why can’t it be solved by disallowing more than one Player and telling the users to simply start another instance of Vassal (ModuleManager)?
I can start two instances of minecraft, each gets its own 1gb heap and everything works fine, there’s no forking needed, me the user is the fork, I start the processes manually. Why can’t Vassal do this? The memory/heap can’t be the reason for starting JVMs in a subprocess, it’s 2020 outside, one of the most popular computer games is written in Java and it’s a 3d game, and it’s an old game too it is about 10 years old by now. Memory issues being the reason for a graphically simple 2d application like Vassal doing these JVM-fork hacks is not an acceptable excuse. 1x ModuleManager + 1x Player, xor 1x ModuleManager + 1x Editor must be possible inside a single JVM, inside a default -Xmx1g even. And if some rogue module needs more, it can tell the users to give the JVM more heap. Minecraft mods do this too and the minecraft players found ways to assign more heap even way before they got this minecraft launcher where the JVM args are neatly shown in a text field. This should be the problem of the module designers anyways, not Vassal’s problem, if the designers write modules that need so much memory they can be bothered to teach their users how to assign more memory to Vassal.
Minor correction, I actually looked up, by now Minecraft is the best-selling video game of all time. And it’s written in Java, and runs on any regular JVM. I can tell it to run on any one of my 10 different JVMs, Oracle’s, openJDK, Adopt, Zulu, Graal. And you’re complaining about heap usage for some 2d application and how bad Java is in general. Wake up, it’s not 1998 and Java 1.3 anymore 