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.

Sign up for GitHub

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

Jump to bottom

Revisiting cmake code for generating coverage reports #2047

Merged
merged 9 commits into from
Jan 6, 2022
Merged

Revisiting cmake code for generating coverage reports #2047

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
54dc568
cmake: better usage of gcovr for coverage reports
piponazo Jan 4, 2022
e276660
Add test for FileIo::setPath
piponazo Jan 4, 2022
e86fecf
Remove useless seek() overload
piponazo Jan 4, 2022
ff8b18c
Add missing override specifiers
piponazo Jan 5, 2022
788ece5
ignore .vs folder
piponazo Jan 5, 2022
b33afe3
Small refactors in BasicIo implementations
piponazo Jan 5, 2022
4f640cb
Remove duplicated doxygen doc
piponazo Jan 5, 2022
098679e
Refactor & add tests for MemIO
piponazo Jan 5, 2022
d6f8649
Fix compilation warnings on windows
piponazo Jan 5, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,4 +23,5 @@ src/doxygen.hpp
test/tmp/*
doc/html
contrib/vms/.vagrant
/.vscode
/.vscode
.vs/
13 changes: 8 additions & 5 deletions cmake/gcovr.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
# Intentd usage
# cmake -DBUILD_WITH_COVERAGE=yes ../
# make -j
# make tests
# make coverage
# Intended usage
# cmake -DCMAKE_BUILD_TYPE=Debug -DBUILD_WITH_COVERAGE=yes ../
# cmake --build . --config Debug
# ctest
# cmake --build . --config Debug --target coverage

if(BUILD_WITH_COVERAGE)
find_program (GCOVR gcovr)
Expand All @@ -12,6 +12,9 @@ if(BUILD_WITH_COVERAGE)
add_custom_command(OUTPUT _run_gcovr_parser
POST_BUILD
COMMAND ${GCOVR} --root ${CMAKE_SOURCE_DIR} --object-dir=${CMAKE_BINARY_DIR} --html --html-details -o coverage_output/coverage.html
--exclude-directories xmpsdk --exclude-directories unitTests --exclude-directories samples
--exclude '.*xmpsdk.*' --exclude '.*unitTests.*' --exclude '.*samples.*'

WORKING_DIRECTORY ${CMAKE_BINARY_DIR}
)
add_custom_target (coverage DEPENDS _run_gcovr_parser)
Expand Down
30 changes: 7 additions & 23 deletions include/exiv2/basicio.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,11 +183,8 @@ namespace Exiv2 {
@return 0 if successful;<BR>
Nonzero if failure;
*/
#if defined(_MSC_VER)
virtual int seek(int64_t offset, Position pos) = 0;
#else
virtual int seek(long offset, Position pos) = 0;
#endif

/*!
@brief Safe version of `seek()` that checks for errors and throws
an exception if the seek was unsuccessful.
Expand All @@ -196,11 +193,7 @@ namespace Exiv2 {
@param pos Position from which the seek should start
@param err Error code to use if an exception is thrown.
*/
#if defined(_MSC_VER)
void seekOrThrow(int64_t offset, Position pos, ErrorCode err);
#else
void seekOrThrow(long offset, Position pos, ErrorCode err);
#endif

/*!
@brief Direct access to the IO data. For files, this is done by
Expand Down Expand Up @@ -263,7 +256,7 @@ namespace Exiv2 {
@note This method should be only called after the concerned data (metadata)
are all downloaded from the remote file to memory.
*/
virtual void populateFakeData() {}
virtual void populateFakeData() = 0;

/*!
@brief this is allocated and populated by mmap()
Expand Down Expand Up @@ -450,11 +443,8 @@ namespace Exiv2 {
@return 0 if successful;<BR>
Nonzero if failure;
*/
#if defined(_MSC_VER)
int seek(int64_t offset, Position pos) override;
#else
int seek(long offset, Position pos) override;
#endif

/*!
@brief Map the file into the process's address space. The file must be
open before mmap() is called. If the mapped area is writeable,
Expand Down Expand Up @@ -672,11 +662,8 @@ namespace Exiv2 {
@return 0 if successful;<BR>
Nonzero if failure;
*/
#if defined(_MSC_VER)
virtual int seek(int64_t offset, Position pos);
#else
int seek(long offset, Position pos) override;
#endif
virtual int seek(int64_t offset, Position pos) override;

/*!
@brief Allow direct access to the underlying data buffer. The buffer
is not protected against write access in any way, the argument
Expand Down Expand Up @@ -957,11 +944,8 @@ namespace Exiv2 {
@return 0 if successful;<BR>
Nonzero if failure;
*/
#if defined(_MSC_VER)
virtual int seek(int64_t offset, Position pos);
#else
int seek(long offset, Position pos) override;
#endif
virtual int seek(int64_t offset, Position pos) override;

/*!
@brief Not support
@return NULL
Expand Down
74 changes: 4 additions & 70 deletions src/basicio.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,11 +84,7 @@ namespace Exiv2 {
enforce(!error(), err);
}

#if defined(_MSC_VER)
void BasicIo::seekOrThrow(int64_t offset, Position pos, ErrorCode err) {
#else
void BasicIo::seekOrThrow(long offset, Position pos, ErrorCode err) {
#endif
const int r = seek(offset, pos);
enforce(r == 0, err);
}
Expand Down Expand Up @@ -628,6 +624,7 @@ namespace Exiv2 {
fileIo->close();
// Check if the file can be written to, if it already exists
if (open("a+b") != 0) {
/// \todo Use std::filesystem once C++17 can be used
// Remove the (temporary) file
#ifdef EXV_UNICODE_PATH
if (fileIo->p_->wpMode_ == Impl::wpUnicode) {
Expand Down Expand Up @@ -898,7 +895,6 @@ namespace Exiv2 {
return putc(data, p_->fp_);
}

#if defined(_MSC_VER)
int FileIo::seek( int64_t offset, Position pos )
{
assert(p_->fp_ != 0);
Expand All @@ -917,24 +913,6 @@ namespace Exiv2 {
return std::fseek(p_->fp_,static_cast<long>(offset), fileSeek);
#endif
}
#else
int FileIo::seek(long offset, Position pos)
{
assert(p_->fp_ != 0);

int fileSeek = 0;
switch (pos) {
case BasicIo::cur: fileSeek = SEEK_CUR; break;
case BasicIo::beg: fileSeek = SEEK_SET; break;
case BasicIo::end: fileSeek = SEEK_END; break;
}

if (p_->switchMode(Impl::opSeek) != 0) {
return 1;
}
return std::fseek(p_->fp_, offset, fileSeek);
}
#endif

long FileIo::tell() const
{
Expand Down Expand Up @@ -1279,7 +1257,6 @@ namespace Exiv2 {
return data;
}

#if defined(_MSC_VER)
int MemIo::seek( int64_t offset, Position pos )
{
int64_t newIdx = 0;
Expand All @@ -1302,30 +1279,6 @@ namespace Exiv2 {
p_->eof_ = false;
return 0;
}
#else
int MemIo::seek(long offset, Position pos)
{
long newIdx = 0;

switch (pos) {
case BasicIo::cur: newIdx = p_->idx_ + offset; break;
case BasicIo::beg: newIdx = offset; break;
case BasicIo::end: newIdx = p_->size_ + offset; break;
}

if (newIdx < 0)
return 1;

if (newIdx > p_->size_) {
p_->eof_ = true;
return 1;
}

p_->idx_ = newIdx;
p_->eof_ = false;
return 0;
}
#endif

byte* MemIo::mmap(bool /*isWriteable*/)
{
Expand Down Expand Up @@ -1919,7 +1872,6 @@ namespace Exiv2 {
src.close();
}

#if defined(_MSC_VER)
int RemoteIo::seek( int64_t offset, Position pos )
{
assert(p_->isMalloced_);
Expand All @@ -1931,32 +1883,14 @@ namespace Exiv2 {
case BasicIo::end: newIdx = p_->size_ + offset; break;
}

/// \todo in previous ::seek() overload there was the following comment:
/// #1198. Don't return 1 when asked to seek past EOF. Stay calm and set eof_
/// And the following line was commented. Check out this part of the code
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is the only change that I think it might be a bit risky. The previous 2 RemoteIo::seek() overloads had slightly different code.

Any ideas about how we could test this? I think somebody added some python tests to exercise the RemoteIo code, but I did not pay too much attention there.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@piponazo I thought @kevinbackhouse removed isMalloced_ from the code base last week.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I removed a different isMalloced_:

exiv2/src/tiffcomposite_int.hpp

Line 549 in c28e501

bool isMalloced_; //!< True if this entry owns the value data

I think the isMalloced_ in basicio.cpp looks ok because I don't see any copy constructors or operators. In TiffEntryBase I was worried that the ownership of the buffer was unclear.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it would be safer to make sure that 0 <= idx_ and idx_ <= size_ are always true. Because code like this doesn't look like it was designed to handle the case where idx_ is out-of-bounds:

exiv2/src/basicio.cpp

Lines 1858 to 1866 in c28e501

long RemoteIo::read(byte* buf, long rcount)
{
assert(p_->isMalloced_);
if (p_->eof_) return 0;
p_->totalRead_ += rcount;
size_t allow = std::min(rcount, (long)( p_->size_ - p_->idx_));
size_t lowBlock = p_->idx_ /p_->blockSize_;
size_t highBlock = (p_->idx_ + allow)/p_->blockSize_;

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I took your suggestion and added the check for 0 <= idx_ and also set the EOF variable to true in case the newIdx is bigger than the size.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Any ideas about how we could test this? I think somebody added some python tests to exercise the RemoteIo code, but I did not pay too much attention there.

@piponazo There was some python to test RemoteIo. I think it was focused on getting complete files and the like and could exercise different http verbs (the request methods: get/head/etc). We've deprecated the WebReady ssh code. I don't know the status of ftp. Nobody has ever reported any issue about ssh or ftp, so I don't think it's being used.

http is tested in the bash tests by powering up the python web-server. There's code in tests/bash_tests/utils.py to start/stop the python web server.

Can you write a C++ unit test (in bin/unit_tests) to test BasicIo::seek() on local storage? When that's working, we can discuss how best to power up the python web-server and perform the test on a remote file.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hi Robin. We already have such unit tests for MemIo and FileIo. You can see those tests in the following files:

exiv2/unitTests/test_basicio.cpp
exiv2/unitTests/test_FileIo.cpp

I have actually revisited some of those tests, made some improvements there (specially in terms of test naming) and add few new tests.

Copy link
Collaborator

@clanmills clanmills Jan 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Careful API analysis and code elimination in classes FileIo and MemIo is of great value. Probably 99.9% of BasicIo use occurs in class FileIo and class MemIo.

How about opening an issue: "Unit Test RemoteIo". Just punt your observations about RemoteIo and one day somebody will have fun dealing with them.

if ( /*newIdx < 0 || */ newIdx > static_cast<uint64_t>(p_->size_) ) return 1;
p_->idx_ = static_cast<long>(newIdx); //not very sure about this. need more test!! - note by Shawn fly2xj@gmail.com //TODO
p_->eof_ = false;
return 0;
}
#else
int RemoteIo::seek(long offset, Position pos)
{
assert(p_->isMalloced_);
long newIdx = 0;

switch (pos) {
case BasicIo::cur: newIdx = p_->idx_ + offset; break;
case BasicIo::beg: newIdx = offset; break;
case BasicIo::end: newIdx = p_->size_ + offset; break;
}

// #1198. Don't return 1 when asked to seek past EOF. Stay calm and set eof_
// if (newIdx < 0 || newIdx > (long) p_->size_) return 1;
p_->idx_ = newIdx;
p_->eof_ = newIdx > static_cast<long>(p_->size_);
if (p_->idx_ > static_cast<long>(p_->size_))
p_->idx_ = static_cast<long>(p_->size_);
return 0;
}
#endif

byte* RemoteIo::mmap(bool /*isWriteable*/)
{
Expand Down
20 changes: 19 additions & 1 deletion unitTests/test_FileIo.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ namespace
{
const std::string testData(TESTDATA_PATH);
const std::string imagePath(testData + "/DSC_3079.jpg");
const std::string nonExistingImagePath(testData + "/nonExisting.jpg");
} // namespace

TEST(AFileIO, canBeInstantiatedWithFilePath)
Expand All @@ -43,10 +44,27 @@ TEST(AFileIO, isOpenDoItsJob)
{
FileIo file(imagePath);
ASSERT_FALSE(file.isopen());
file.open();
ASSERT_EQ(0, file.open());
ASSERT_TRUE(file.isopen());
}

TEST(AFileIO, failsToOpenANonExistingFile)
{
FileIo file(nonExistingImagePath);
ASSERT_FALSE(file.isopen());
ASSERT_EQ(1, file.open());
ASSERT_FALSE(file.isopen());
}

TEST(AFileIO, canChangeItsPathWithSetPath)
{
FileIo file(nonExistingImagePath);
ASSERT_EQ(nonExistingImagePath, file.path());
file.setPath(imagePath);
ASSERT_EQ(imagePath, file.path());
}


TEST(AFileIO, returnsFileSizeIfItsOpened)
{
FileIo file(imagePath);
Expand Down