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

Download and use 16-bit SkiFree for NE tests #90

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

Conversation

disinvite
Copy link
Collaborator

Resolves #3. We sort of landed on using existing binaries to test the binary image processing rather than concocting examples ourselves, at least for now.

SkiFree needs no introduction and the EXE is small. To test locally you can get the 16-bit version from Chris Pirih's site, from archive.org, or some old floppy disk you have on hand.

This fixes a bug with NE header unpacking and adds some rudimentary tests. I didn't expand the functionality yet because I'm not sure how we should handle 16-bit addresses.

We cache the download but the cache is not shared between branches so maybe using archive.org is not the best thing long-term. As we add more sample binaries we may want to parameterize the CI task and the way we load the files using pytest. Is that something we should figure out now or later?

Copy link
Collaborator

@jonschz jonschz left a comment

Choose a reason for hiding this comment

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

Looks good! Only one small detail: Maybe we want to add some canonical directory to the .gitignore if one wants to keep the files there locally for testing? We could also change the CI scripts to download them there. I'd still keep the option to specify the files by path if one wants to keep them separate.

@disinvite
Copy link
Collaborator Author

Good idea! I'll look into that and see if we can add it here. It would make adding more sample binaries easier.

Should that directory just be under /tests? We could maybe try to autoload it unless you specify an alternate location at the command line.

@jonschz
Copy link
Collaborator

jonschz commented Mar 1, 2025

How about tests/binaries or something along these lines? Looking there by default sounds like a great idea, it'll simplify IDE integration. Anything will do as far as I am concerned, no need to wait for another approval.

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.

New binary files for unit tests
2 participants