Skip to content
Permalink
Browse files

Fix for PyQgsVectorFileWriter test segfault on Mac

- 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 89eb054d44613f60075bbc14cca958a8ad605d4f
Showing with 4 additions and 2 deletions.
  1. +4 −2 tests/src/python/test_qgsvectorfilewriter.py
@@ -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(
@@ -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

This comment has been minimized.

Copy link
Member Author

@dakcarto dakcarto replied Nov 21, 2012

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

This comment has been minimized.

Copy link
Member

@timlinux timlinux replied Nov 22, 2012

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

This comment has been minimized.

Copy link
Member Author

@dakcarto dakcarto replied Nov 22, 2012

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

This comment has been minimized.

Copy link
Member

@timlinux timlinux replied Nov 22, 2012

Please sign in to comment.
You can’t perform that action at this time.