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 generate_fingerprint_bytes #6349

Conversation

AllSeeingEyeTolledEweSew
Copy link
Contributor

Closes #5985

lt.generate_fingerprint_bytes(b"A", 1, 2, 3, 4), # type: ignore
b"-A\x001234-",
lt.generate_fingerprint_bytes(b"A", 1, 2, 3, 4),
b"---1234-",
Copy link
Owner

Choose a reason for hiding this comment

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

this doesn't look right. what happened to the A?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The original test was wrong. The behavior of generate_fingerprint_bytes() is analogous to generate_fingerprint() as expected.

The tests I wrote with @unittest.skip() were not all correct (because they wouldn't run, so I couldn't verify the desired condition would be correct until the fix was made).

I expect that here:

  • I wrote and validated the test for fingerprint() (str(fingerprint("A", 1, 2, 3, 4)) == "-A\x001234-")
  • I copied that output into some test cases for generate_fingerprint()
  • I realized we should have generate_fingerprint_bytes()
  • I split the functions into test_generate() (with @unittest.skip()) and test_generate_str()
  • I validated test_generate_str() (generate_fingerprint("A", 1, 2, 3, 4) == "---1234-", unlike fingerprint())
  • I forgot to analogously update test_generate()

Copy link
Owner

Choose a reason for hiding this comment

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

I expect the "A" that's passed it would be used in the final fingerprint. It looks like a bug that it turns into -- instead. I can take a closer look at what's going on

Copy link
Owner

Choose a reason for hiding this comment

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

I see, there's a check and the name has to be two bytes long

@arvidn arvidn merged commit b7d5ac9 into arvidn:master Aug 2, 2021
@AllSeeingEyeTolledEweSew AllSeeingEyeTolledEweSew deleted the asetes-generate-fingerprint-bytes branch August 2, 2021 17:42
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