Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Running a test suite sometimes aborts #2

Closed
mbarchfe opened this issue Feb 5, 2014 · 11 comments
Closed

Running a test suite sometimes aborts #2

mbarchfe opened this issue Feb 5, 2014 · 11 comments

Comments

@mbarchfe
Copy link
Contributor

mbarchfe commented Feb 5, 2014

Hello,

running a test suite sometimes aborts if the data event is fired with only a part of the data written from the SLIM client.

Below is a possible patch. However, checking for completeness ist not perfect. isComplete should actually try to deserialize the message and only if that succeeds continue.

diff --git a/node_modules/decaf/src/server.coffee b/node_modules/decaf/src/server.coffee
index 0055f0b..7d2ef39 100644
--- a/node_modules/decaf/src/server.coffee
+++ b/node_modules/decaf/src/server.coffee
@@ -10,14 +10,24 @@ class exports.Server
   listen: (@socket) =>
     @processor = new Processor @socket
     @socket.write @hello
+    @old_data = ''
     @socket.on 'data', @message
-
-  message: (data) =>
-    command = @command_from data

-
-    if command is 'bye'
-    then @exit()
-    else @processor.run command
+    
+  isComplete: (data) ->
+    lastChars = data[-2..]
+    return lastChars==":]" or lastChars=="ye"         
+
+  message: (new_data) =>
+    data = @old_data + new_data 
+    if not this.isComplete(data)
+        @old_data = data
+    else
+        @old_data = ''
+        command = @command_from data
+
+        if command is 'bye'
+        then @exit()
+        else @processor.run command

   command_from: (data) -> data[7..].toString()
@msuarz
Copy link
Member

msuarz commented Feb 5, 2014

wow i haven't been around this code for a bit ... please provide a test example that fails ... regardless i'll look into it tonight

@mbarchfe
Copy link
Contributor Author

mbarchfe commented Feb 5, 2014

First of all, thanks for providing this project. It was very useful for us.
Concerning a valid test example: I guess I would have to give you our whole test suite. The problem is, that even the same suite sometimes runs and sometimes not (roughly one out of five test runs fail).
Probably it would be best to write a test with a SLIM client which sends part of the command, flushes and then sends the other. In our test suite the failing part was always same: one larger table where the serialised command exceeded 8192 bytes. Sometimes the on data event only delivered the first 8192 bytes.

@msuarz
Copy link
Member

msuarz commented Feb 6, 2014

cool got it ... i'll mess with an echo test passing a long string

@msuarz
Copy link
Member

msuarz commented Feb 6, 2014

your code is right on the money ... but i'll implement it based on the expected length of the command ... the first 6 digits ... it shouldn't take long to do

@msuarz
Copy link
Member

msuarz commented Feb 7, 2014

Hey mbarchfe .. I just released a new version with the fix ... please let me know if it works to close the issue

Thank you
Mike

@mbarchfe
Copy link
Contributor Author

mbarchfe commented Feb 9, 2014

Hi Mike,

Thanks for the patch. Unfortunately, it did not work with umlaute. I have created a pull request with a fix and further explanations. Also, one test was broken and I have added new tests for the umlaut-stuff.

BTW: could you describe somewhere how to run the complete test suite? I ran it with the command below, but that seems awkward.
Thanks
Markus

(cd ../src; coffee -c *.coffee); mocha slim_chunk_buffer_spec.js server_spec.js command_spec.js calls_spec.js import_spec.js serialize_response_spec.js

@msuarz
Copy link
Member

msuarz commented Feb 10, 2014

ok i merged the pull request ... did some cleanup and re-org ... i didn't tuned package.json 100% yet but the idea is to do npm install and then npm test ... that leaves u in autotest mode ... u could just run mocha from the main dir now if u dont want the autotest stuff (u need the latest coffeescript installed globally)

now back to the issue ... i don't think it is fixed ... i tried a test with an umlaut n' it is crashing badly ... please notice the unit tests are not that interesting ... the important tests are in slim ... for that u could c the startFitNesse batch n' get fitnesse running ... once in there u could check out this test ... http://localhost:8080/ExampleS.EchO ... in particular the piece ...

|script|Echo |
|echo|echöe|

... so yeah i guess the response serializer needs to count utf-8 properly too ... i'll work on it tomorrow

Thank you very much
Mike

@mbarchfe
Copy link
Contributor Author

Hi,

thanks for restructuring the tests to run with mocha. I prefer to run them that way.

The serialised issue is funny: it seems as if the slim client does not handle incoming utf-8 in the same way as outgoing. I have implemented a serialiser which counts 2 for every utf-8 char, but the java slim deserialiser uses string.substring() to cut the string and that counts 1 for every utf-8 char.
When fiddling around with the SLIM to java I also found that echö counted as 4 bytes.
Sorry, too late now to figure it out.

cheers
Markus

@msuarz
Copy link
Member

msuarz commented Feb 12, 2014

Ok thank you for the excellent patch. It was merged and i did some cleanup around the serializer area ... i still don't know why i got 2 classes in 1 file n' exported as functions ... bahh experimenting ... anyways a new version was released please let me know if it solves the problem.

Thank you very much for your code and time dedicated to this issue
Vielen dank
Mike

@mbarchfe
Copy link
Contributor Author

Vielen Dank auch Dir, Mike.

One final issue: when I reinstalled via npm I found a lot of new files, like startFitNesse.bat and all the test files. was that your intention?

@msuarz
Copy link
Member

msuarz commented Feb 18, 2014

nahh i wasnt neat enuf ... just pushed a cleaner version ... danke schoon

@msuarz msuarz closed this as completed Feb 18, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants