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

Feature/random test output #9

Merged
merged 5 commits into from
Jan 15, 2016

Conversation

psorowka
Copy link
Contributor

When a random test package triggers a unhandled parser exception, the
content of this packet as well as the stack trace are printed in order
to ease debugging

Peter Sorowka added 3 commits January 15, 2016 12:33
When a random test package triggers a unhandled parser exception, the
content of this packet as well as the stack trace are printed in order
to ease debugging
@mcollina
Copy link
Member

Are you ok to help maintaining this module? If you are ok with CONTRIBUTING.md, I'll add you to the collaborators.


function doParse () {
var parser = mqtt.parser()
parser.on('error', onError)
parser.parse(crypto.randomBytes(Math.floor(Math.random() * 10)))
randomPacket = crypto.randomBytes(Math.floor(Math.random() * 10))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Possibly we should generate more longer packets, maybe up to 256 or 512 bytes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also thought we possibly should shape the stochastic distritbution of the first byte so that most packets at least enter the parser and aren't rejected by the first gate (packet type selector)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That make sense as well :).

@psorowka
Copy link
Contributor Author

Sure!

@mcollina
Copy link
Member

Feel free to add yourself to the contributors list at the end :).

Peter Sorowka added 2 commits January 15, 2016 17:37
- longer chunks are passed into the parser, causing multiple packets
  parsed from one chunk from time to time
- first byte of every chunk is now very likely to be valid, thus more
  packets actually make it to the deeper parsing routines
- more detailed test output
@psorowka
Copy link
Contributor Author

I added everything we talked about and observed a few interesting things:

  • first I added a counter on the packet event
  • when max. chunk length was set to 10, the number of chunks that caused either an error or a packet was way below 10%, assumingly because most packets just where shorter than the suggested packet length
  • on the other hand when setting the max chunk length to 512 or even 1024 lead to way more (packets + errors) than chunks, assumingly because multiple "packets" could be found in some chunks
  • this leads to the behavior that increasing chunk size enormously increases the test-time.

I would finish and merge this test now.

@mcollina
Copy link
Member

How long is now? I've set things up we run this on travis, but if that takes much time we need to short that up / make the number of rounds configurable, so in Travis is small enough.

@mcollina
Copy link
Member

All good for me, feel free to merge.

@psorowka
Copy link
Contributor Author

Duration should be fine for travis. It was more like: increasing packet length from 10 to 512 increased the time from 0.8s to 2.2s. In the first place I thought: wow, why should packet length increase the execution time? But the answer can be seen above.

psorowka pushed a commit that referenced this pull request Jan 15, 2016
Improved algorithm and output for parser tests of random chunks
@psorowka psorowka merged commit 6d23419 into mqttjs:master Jan 15, 2016
@psorowka psorowka deleted the feature/randomTestOutput branch January 15, 2016 17:07
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

Successfully merging this pull request may close these issues.

2 participants