Skip to content

Commit

Permalink
Fix for PyQgsVectorFileWriter test segfault on Mac
Browse files Browse the repository at this point in the history
- Initialize newFilename to QString(), or QgsDebugMsg for newFilename segfaults (line 650)
- Set test assert to QgsVectorFileWriter::WriterError enum for success (NoError = 0, i.e. false)
  • Loading branch information
dakcarto committed Nov 21, 2012
1 parent 52e6c50 commit 89eb054
Showing 1 changed file with 4 additions and 2 deletions.
6 changes: 4 additions & 2 deletions tests/src/python/test_qgsvectorfilewriter.py
Expand Up @@ -67,6 +67,7 @@ def testWrite(self):
myLayerOptions = QStringList()
mySelectedOnlyFlag = False
mySkipAttributesFlag = False
myNewFileName = QString()
myGeoCrs = QgsCoordinateReferenceSystem()
myGeoCrs.createFromId(4326, QgsCoordinateReferenceSystem.EpsgCrsId)
myResult = QgsVectorFileWriter.writeAsVectorFormat(
Expand All @@ -79,8 +80,9 @@ def testWrite(self):
myErrorMessage,
myOptions,
myLayerOptions,
mySkipAttributesFlag)
assert myResult==True
mySkipAttributesFlag,
myNewFileName)
assert myResult==QgsVectorFileWriter.NoError

This comment has been minimized.

Copy link
@timlinux

timlinux Nov 22, 2012

Member

assert myResult == True <-- pep8 friendly

This comment has been minimized.

Copy link
@timlinux

timlinux Nov 22, 2012

Member

assert myResult == QgsVectorFileWriter.NoError


if __name__ == '__main__':
unittest.main()

4 comments on commit 89eb054

@dakcarto
Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure if these edits are enough to warrant removing @expectedfailure decorator. I looked on CDash and it seems that it may fix similar failures on other platforms.

When running with debug output, if the new optional parameter newFilename is unused and not initialized in QgsVectorFileWriter constructor, won't the QString::append() on line 650 will still segfault when trying to dereference the uninitialized pointer?

@timlinux
Copy link
Member

Choose a reason for hiding this comment

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

Hi Larry

I wrote this test because code in my InaSAFE realtime which used to work fine suddenly brok (I think after a commit from Radim 2 weeks back).

I think it is better that we fix the underlying code, unless we are going to consider this to be an API breakage (which I dont see the need for).

Regards

Tim

@dakcarto
Copy link
Member Author

Choose a reason for hiding this comment

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

Hi Tim,

The follow-up 6b3aed0 commit fixes the underlying code for both versions of the test. (Not sure if it fixes all broken aspects of the code.)

@timlinux
Copy link
Member

@timlinux timlinux commented on 89eb054 Nov 22, 2012 via email

Choose a reason for hiding this comment

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

Please sign in to comment.