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

FR: add tests that assert equality of bytestreams that take a roundtrip through psdtags with unknown=False #11

Closed
widdowson opened this issue Feb 22, 2024 · 2 comments

Comments

@widdowson
Copy link

I think it would be beneficial to add tests that check for non-determinism and/or accidental misparsing/mutation. If not at the highest/largest levels, at least for smaller primitives like PsdLayer etc, using existing test inputs.

I did this manually and fell down a rabbit hole unearthing what seems to be issue #10. I think such a test feature would likely catch small and large regressions across the codebase, either retroactively or in the future.

That being said, I don't have a sufficient command of the Adobe spec to know as to whether there are situations where non-determinism is encouraged (!!), nor of your codebase to know if determinism and repeatability are inherit goals. So please forgive me if I'm quite offbase here.

Thank you!
-Andrew

@cgohlke
Copy link
Owner

cgohlke commented Feb 22, 2024

You are right. All the test files that could have caught #10 on my system were skipped by unknown=False.

@cgohlke
Copy link
Owner

cgohlke commented Feb 22, 2024

Even at the lowest level, for example Unicode strings, I was unable to achieve determinism. The strings are supposed to be terminated by two null bytes, except sometimes they are not. I'm not sure the current implementation is according to specification, but so far no-one complained.

Testing could definitely be improved. However, given the lack of time, access to Photoshop, anything "ground truth" to test against (like a reference implementation or dataset), and reliable specification, I am afraid the current state is as good as it gets for now.

@cgohlke cgohlke closed this as completed Feb 22, 2024
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