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 tests #3

Closed
wants to merge 7 commits into from
Closed

Add tests #3

wants to merge 7 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Mar 4, 2015

I added bunch of regression tests. There are a few that aren't currently passing, I left them commented out so the tests would succeed overall. (I couldn't find a "TODO" test mechanism in testthat so I opened a feature request for it.)

@ghost
Copy link
Author

ghost commented Mar 4, 2015

I also have a "FixBugs" branch that picks up where this one leaves off, fixing the problems I found in the tests:

kenahoo/pack@AddTests...kenahoo-windlogics:FixBugs

If you approve this PR I can rebase my FixBugs branch on top of it for a cleaner merge.

@joshuaulrich
Copy link
Owner

Thanks! How did you create the raw vectors you're testing against?

Regarding your "FixBugs" branch: can you please provide more details in the commit message about what is being fixed? I try to follow these suggestions, paying special attention to explaining the problem and the solution.

@kenahoo
Copy link
Contributor

kenahoo commented Mar 5, 2015

To find the appropriate raw vectors, sometimes I used Perl (in those cases I pasted the Perl code as comments into the tests), sometimes just mental conversion between decimal & hex, I guess. I can try to make them more explicit.

@joshuaulrich
Copy link
Owner

This has been merged into the develop branch. Note that I made a few changes and squashed all the commits into a single commit.

@ghost
Copy link
Author

ghost commented Mar 10, 2015

Good point about the native format. Looks like we can check .Platform$endian to be aware of the machine type.

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