Skip to content

Conversation

@maxhora
Copy link
Contributor

@maxhora maxhora commented Jun 14, 2017

No description provided.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

MSVC defines off_t as

typedef long _off_t; // file offset value

msvc long type width is 32 bits and it's not enough to keep 64 bits of size_t(unsigned long long).
File size over 4Gb needs more than 32 bits, that caused the issue.

Copy link
Member

Choose a reason for hiding this comment

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

Do you need to write data to the file for the test to pass?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@wesm , it might be good to test that the whole range of allocated bytes in memory mapped file is writable/readable (since mmap implementation contains logic to calculate file offset and size independently). If that's fine I will change the unit test name from ReadOver4GbFile to ReadWriteOver4GbFile.

Copy link
Member

Choose a reason for hiding this comment

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

This needs to go in a TearDown method so it happens on failure also

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That adds file to be deleted on TearDown

@maxhora
Copy link
Contributor Author

maxhora commented Jun 15, 2017

@wesm , have updated changes, thanks!

@wesm
Copy link
Member

wesm commented Jun 15, 2017

This test takes about 5 seconds to run on my machine with a very fast flash drive, so I don't think we should run it by default. Can you add DISABLED_ to the test name per https://github.com/google/googletest/blob/master/googletest/docs/V1_7_AdvancedGuide.md#temporarily-disabling-tests? We can always run the test with io-file-test --gtest_also_run_disabled_tests

@maxhora
Copy link
Contributor Author

maxhora commented Jun 15, 2017

@wesm , okay, sure, have updated the test to be disabled by default, thanks

@wesm
Copy link
Member

wesm commented Jun 16, 2017

Appveyor build here https://ci.appveyor.com/project/MaxRisuhin/arrow/build/1.0.80. +1, thanks @Maxris!

@asfgit asfgit closed this in 3f26dfa Jun 16, 2017
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