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 simple unit tests for binary files #826

Merged
merged 7 commits into from
Mar 19, 2021
Merged

Add simple unit tests for binary files #826

merged 7 commits into from
Mar 19, 2021

Conversation

emjotde
Copy link
Member

@emjotde emjotde commented Mar 4, 2021

Description

Add a few unit tests for binary files

Checklist

  • I have tested the code manually
  • I have run regression tests
  • I have read and followed CONTRIBUTING.md
  • I have updated CHANGELOG.md

@emjotde emjotde changed the title Mjd/binary test Add simple unit tests for binary files Mar 4, 2021
@emjotde
Copy link
Member Author

emjotde commented Mar 4, 2021

To be merged before #825

@emjotde emjotde requested a review from snukky March 4, 2021 04:52
@snukky
Copy link
Member

snukky commented Mar 4, 2021

Are these unit tests OS and CPU architecture independent?
They fail on Windows: https://github.com/marian-nmt/marian-dev/pull/826/checks?check_run_id=2028168848
If they are CPU dependent, we might need to make some expected outputs dependent on specific SIMD as we do in regression tests. We don't control what machine we get on Azure/GitHub DevOps.

To be merged before #825

This is the opposite what is in #825, could you edit one of your messages?

@emjotde
Copy link
Member Author

emjotde commented Mar 10, 2021

They write and read. I will try to test it on windows to see what is breaking there. Unless you have the possibility now to take a look.

Copy link
Member

@snukky snukky left a comment

Choose a reason for hiding this comment

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

The issue seemed to be TemporaryFile not working properly on Windows because of missing assignment of file_, which was not related to changes from the PR. After 3eca5ed, new unit tests pass.

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