[ERROR] Failures:
[ERROR] ProcessCallableTest.testNormal:70 array lengths differed, expected.length=39 actual.length=40
[ERROR] TailerTest.testTailer:95 expected:<…ailer by TailerTest.[
It has several lines.]> but was:<…ailer by TailerTest.[
It has several lines.
]>
Whereas Eclipse (after several cleans, of course) fails with:
"Plugin execution not covered by lifecycle configuration: org.apache.maven.plugins:maven-checkstyle-plugin:3.1.1:check (execution:validate, phase:validate)
This after several days “out of the pool” while I was working on module maintenance stuff – came back, did the pulling-stuff-down-drill, tried to follow the “New Contributor Workflow”, but this is before even changing any code.
Well it’s good to have the build server, it says the build is ok so the build is ok, what you’re saying must be some kind of local problems.
Let’s see…
This unit test is not OS-independent, it fails on windows due to the line endings having a different length of bytes, but Joel already fixed this in github.com/vassalengine/vassal/pull/55, it just needs to get merged.
Looks like the same problem, the way the Tailer interprets line endings is different from how IOUtils.toString() interpret them, on Windows. Will need to be fixed.
An all-time favorite of the Eclipse Maven (m2e) plugin. There are various solutions for this, some requiring to add special eclipse maven plugins to the pom.xml. I’d rather not do this just to cure problems that are problems of Eclipse or its m2e plugin. It mentions checkstyle, did you update the project configuration (alt+f5)? We have checkstyle now, when I refreshed the project config in Eclipse, the m2e picked up the checkstyle thing and asked me whether I want to install the checkstyle plugin for Eclipse, did it ask you anything like that? If it asks, just install that plugin. And/or do the usual rain dance, a couple times maven → update project configuration, a couple times maven → update dependencies, some project cleans, maybe a couple of Eclipse restarts.
Now for the more mysterious ones.
Where did this come from
We have no bin/ in the source code, Maven puts the compiled stuff under target/, the Makefile works with tmp/. Didn’t Eclipse use bin/ to put the compiled classes, but only in non-maven projects? Might be this Eclipse thing in combination with a (slightly) messed up Maven configuration, feel free to add bin/ to the .gitignore file in the “Eclipse” section and make a PR.
The IOUtilsTest writes this file while running, and deletes it after it’s done. Or at least tries to, apparently not always with success. Probably makes sense to replace “final File ofile = new File(“src/test/resources/IOUtilsTest.out”);” with “final File ofile = new File(“target/test-classes/IOUtilsTest.out”);” to make Git ignore it and Maven clean it on a “mvn clean”. I’ll cook up a PR.
Same story as above, a unit test and its File.delete() did not work for some reason. I’ll change it to create this temporary test file somewhere under target/.
The fun part is, none of this is new, these errors only surface now due to the Maven setup and the unit tests being properly executed on the developers machines. Before that no one (on a Windows computer) seems to have bothered running the unit tests
This unit test is not OS-independent, it fails on windows due to the
line endings having a different length of bytes, but Joel already fixed
this in github.com/vassalengine/vassal/pull/55[1], it just needs
to get merged.
It’s merged now.
“Cattlesquat” wrote:
[ERROR] TailerTest.testTailer:95 expected:<…ailer by TailerTest.[
It has several lines.]> but was:<…ailer by TailerTest.[
It has several lines.
]>
Looks like the same problem, the way the Tailer interprets line endings
is different from how IOUtils.toString() interpret them, on Windows.
Will need to be fixed.
Looking at this now.
src/test/resources/IOUtilsTest.out
The IOUtilsTest writes this file while running, and deletes it after
it’s done. Or at least tries to, apparently not always with success.
Probably makes sense to replace “final File ofile = new
File(“src/test/resources/IOUtilsTest.out”);” with “final File ofile =
new File(“target/test-classes/IOUtilsTest.out”);” to make Git ignore it
and Maven clean it on a “mvn clean”. I’ll cook up a PR.
We have no bin/ in the source code, Maven puts the compiled stuff under
target/, the Makefile works with tmp/. Didn’t Eclipse use bin/ to put
the compiled classes, but only in non-maven projects? Might be this
Eclipse thing in combination with a (slightly) messed up Maven
configuration, feel free to add bin/ to the .gitignore file in the
“Eclipse” section and make a PR.
Please don’t add bin/ to .gitignore unless we know for certain that
Eclipse still creates it post-Maven.
About the TestTailer problem, what needs fixing there is the whole logging setup, not just the unit test.
It seems a unix guru was at work there and reimplemented the “tail” command in a Java class, only to scan the log generated by the logger and copy the log statements into the GUI.
The proper solution for this is to write a custom appender for the logging framework, then simply call log.error() log.warn() log.info() log.debug() and the messages will appear both in the logfile and in the GUI.
Another question is of course, why write the application log, with all the stacktraces that it possibly contains, into the GUI, and scare the hell out of the users…
Excellent, thank you! Working better now, couple specific things below:
Sounds good, I will stop worrying about that one.
TL;DR: Got it working! Thanks! Couldn’t have done it without you!
For posterity (i.e. others in same boat later on), here are some details:
(1) Yes I had done at least one “full round of raindance”
(2) But no, it hadn’t picked up the checkstyle thing.
(3) Reading the above I went to the Maven “install plugin” menu and tried to get it interested in a checkstyle plugin, but although it seemed aware that such a thing existed, double clicking it didn’t install it or anything.
(4) After two more rounds of raindance (gotta get my karma high enough!), I noticed that the error was being marked “in” pom.xml, and on investigating it was on a specific line in the xml (just a “” block in a great wall of xml, but I guess the one that was running the checkstyle). When I rolled over the actual line (the word “execution”), the rollover menu offered me a “Quick Fix™” of “discover new m2e connections”. Because “why the hell not” I said okay, do it. This launched a QUITE lengthy 5-10 minute process of waiting for things, occasionally pressing “next”, and eventually accepting some tasty “terms” and getting to install what I imagine was the checkstyle plugin. This then automatically performed another round of raindance FOR ME, and after it restarted Eclipse then suddenly (or technically after a minute or two) it worked!
Will do! It looks like possibly Eclipse decided that would be a good place to put the binaries it builds for the project. I’m sure I just clicked “Next” on some default-o-rama box at some point.
Haha lol. Yes I am intensely aware that I’m one of the guinea pigs in this operation, but I’m a willing one because I definitely think it’s good to have the development side be accessible to “medium core” developers like me. I am always happy to help work out the kinks. Since if there’s a way to install a dev environment the wrong way, I will definitely find it. Too many years of having nice-people-who-worked-for-me to come and take all the friction out of that side of things…
The PR from the bug I found earlier today is up, and I’ll go get after (git after?) the .gitignore for bin/
About the TestTailer problem, what needs fixing there is the whole
logging setup, not just the unit test.
It seems a unix guru was at work there and reimplemented the “tail”
command in a Java class, only to scan the log generated by the logger
and copy the log statements into the GUI.
The proper solution for this is to write a custom appender for the
logging framework, then simply call log.error() log.warn() log.info()
log.debug() and the messages will appear both in the logfile and in the
GUI.
Another question is of course, why write the application log, with all
the stacktraces that it possibly contains, into the GUI, and scare the
hell out of the users…
This is used by the window which is for displaying the errorLog, which
is under Help > Show Error Log. We found we had much more success at
asking users to look there for the errorLog than trying to explain where
to find it in their file system, and also there’s no good way to tail
a file on Windows, so it’s useful for Windows users who want to see
what’s going into the errorLog as it happens.
If you want to replace the Tailer and adjust LogPane to use it, knock
yourself out, but the replacement had better be simpler than what we
have now.
Note that that I wrote Tailer five years before that answer in SO even
existed. I have no idea if all the things it’s using were available then.
Whoops I got behind on reading this rapidly evolving thread while also typing a response for it, and so by the time I saw Joel asked to hold off adding bin/ to .gitignore I’d already made a PR with it. So I guess just hold off merging that.
I can tell you for sure that it still exists for me now that I’ve run all the maven stuff.
[ERROR] TailerTest.testTailer:95 expected:<…ailer by TailerTest.[
It has several lines.]> but was:<…ailer by TailerTest.[
It has several lines.
]>
Looks like the same problem, the way the Tailer interprets line endings
is different from how IOUtils.toString() interpret them, on Windows.
Will need to be fixed.
I’m not convinced that’s the problem. I suspect it’s a character set
issue instead. Charset.defaultCharset() should be UTF-16 on Windows,
but the input file is ASCII.
The current solution is good enough since it gets the job done.
The custom appender would not necessarily be simpler code-wise than the Tailer, but this example I linked to is too much for us as it uses various formatting extras for different log levels, our solution would certainly be simpler than this, we just need to write plain text into the log panel. Performance-wise I’m not sure but I think sending text in-memory to the logfile and to the panel is faster than sending it to the logfile, then reading from the same logfile and sending the text to the panel. Certainly sounds like much less IO. And from a design perspective it’s cleaner, we would just use the log facility that we already use anyways and tell it to write the log to an additional target.
I’ll make a note in my mental todo-list to implement this some day.
On a side note, these logging appenders are quite powerful, in a bigger project I’ve worked on we used custom appenders to send the log entries, in addition to the regular logfile, to a central logging aggregation server, which put them all in a DB and allowed is to get neat reports by project, module, package, class, timestamp, loglevel etc.
I think it has to do with line endings, I was able to break that unit test on my linux machine by changing the line endings in the input file from linux-LF to windows-CRLF. The diff between actual and expected values showed one of the strings with a 3rd empty line and the other with only 2 lines with text and no 3rd empty line.
Whoops I got behind on reading this rapidly evolving thread while also
typing a response for it, and so by the time I saw Joel asked to hold
off adding bin/ to .gitignore I’d already made a PR with it. So I guess
just hold off merging that.
I can tell you for sure that it still exists for me now that I’ve run
all the maven stuff.
No, Eclipse does not create a bin/ file post-maven, classes are compiled to target/ as expected. However, I have noticed that if you convert a project to maven, eclipse does not delete an existing bin/ folder and you have to delete it manually.
I think it has to do with line endings, I was able to break that unit
test on my linux machine by changing the line endings in the input file
from linux-LF to windows-CRLF. The diff between actual and expected
values showed one of the strings with a 3rd empty line and the other
with only 2 lines with text and no 3rd empty line.
Obviously that would break the test. The file doesn’t have any CR in
it as-is, though.
I remember now why you’ll have a problem replacing the Tailer with a log handler: It’s because the errorLog is written to by the Module Manager, the Player, and the Editor. How will you get the log entries from the other processes?
