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

Enables Windows and Linux build test #304

Merged
merged 3 commits into from
Nov 30, 2023
Merged

Conversation

cheqianh
Copy link
Contributor

@cheqianh cheqianh commented Nov 20, 2023

This PR enables build tests on Windows and Linux and unblocks the current release pipeline issue.

In details, this PR

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@cheqianh
Copy link
Contributor Author

cheqianh commented Nov 27, 2023

Comment on lines 17 to 21
size = _minimal_spec.get_input_file_size()
if platform.system() == 'Windows':
assert size == 167
else:
assert size == 161
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please add some commentary to explain this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added, the sample file cat.ion has different file size according to the operating systems.

Comment on lines 17 to 23
size = _minimal_spec.get_input_file_size()
# Different operating systems use different file structures, which, as a result, leads to varying amounts of
# space being used for storing files.
if platform.system() == 'Windows':
assert size == 167
else:
assert size == 161
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we gaining anything by asserting the apparently platform-dependent size, as opposed to, e.g., just the name of the file?

Copy link
Contributor Author

@cheqianh cheqianh Nov 29, 2023

Choose a reason for hiding this comment

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

Discussed offline, comparing the returned size against the expected size retrieved by the method Path(join(_generate_test_path("sample_spec"), _minimal_params['input_file'])).stat().st_size

where join(_generate_test_path("sample_spec"), _minimal_params['input_file']) indicates cat.ion

@cheqianh
Copy link
Contributor Author

Tests passed. Only few failures are due to the performance regression variance

@cheqianh cheqianh merged commit ea8bdf2 into amazon-ion:master Nov 30, 2023
36 of 42 checks passed
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