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

Add JUnit and unit tests for VariableLengthInt #24

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

pearcemerritt
Copy link

@pearcemerritt pearcemerritt commented Aug 21, 2019

I came across this library as something I might use in an android app needing MIDI support. At some point I started digging into the library to debug some problems in my own code and was puzzled by a few things, especially VariableLengthInt. I thought it might be instructive to write some tests for it to run through some use cases and figure out what it was doing. And, of course, perhaps it would end up something I could contribute back to the library. Maybe, unlikely as it may be, someone else who is in my shoes down the road will look for unit tests as a form of documentation (as I sometimes do).

Eventually, I became overwhelmed by the oddities of how midi file creation logic seemed to be implemented and had to dig into the official specs. That cleared up a lot haha.

I went ahead and rounded out the test suite I had started and in the process, found a small inconsistency in how VariableLengthInt stores state. I went ahead and fixed that in this PR along with the test suite, but could also split the changes into multiple PR's if that feels like a better approach.

Here's a description of the inconsistency:
Seems like (without the commit in this PR) when a VariableLengthInt object is created with the stream constructor, it doesn't store the bytes as they are read from the stream: it removes the most significant digit so that integer math is correct later. The int constructor, on the other hand, does store the bytes with the most significant bits set to properly represent a variable length quantity (all bytes have a 1 in their MSB except for the last byte which has a 0). This is likely (hopefully) not an issue for anyone. By searching some reference chains in my IDE, it seems like VariableLengthInt.getBytes() only ever really gets called in two situations:

  1. by MetaData objects storing data length
  2. by MidiTrack objects storing delta time values

The first situation likely is not an issue because the length of the data seems to be always gotten from the length of a byte array (hence using the int constructor of VariableLengthInt). The second situation shouldn't be an issue because the event parsing code throws away the VariableLengthInt object that it creates after reading from the stream and only passes in the integer value to the function signature that expects a long for the delta. That long seems to inevitably get passed up the hierarchy chain to MidiEvent, which finally passes it to the VariableLengthInt's int constructor. So, unless I'm missing something, all code paths that get bytes from a VariableLengthInt involve objects created with the int constructor. However, if any 3rd party code is relying on the underlying implementation of VariableLengthInt, this would be a breaking change. I've bumped the version in the pom.xml to 1.1 for good measure, but wouldn't object to a major version bump (doesn't look like there's any release process in place, so hopefully version numbers are cheap :P ).

… and fixed small inconsistency in state of VariableLengthInts created by different constructors
<maven.compiler.source>1.5</maven.compiler.source>
<maven.compiler.target>1.5</maven.compiler.target>
<maven.compiler.source>1.7</maven.compiler.source>
<maven.compiler.target>1.7</maven.compiler.target>
Copy link
Author

Choose a reason for hiding this comment

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

oh yea, I had to do this initially in order to build the artifact, but would be happy to revert this if it's inappropriate for this changeset :)

@dejankos
Copy link

dejankos commented Nov 2, 2020

I don't think anyone is maintaining this project anymore... I had to do the exact same thing to build from source

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