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

[processing] check for parent alg dependencies in hasDependencies() #3742

Merged
merged 1 commit into from
Nov 12, 2016

Conversation

nirvn
Copy link
Contributor

@nirvn nirvn commented Nov 10, 2016

@volaya , this PR fixes hasDependencies() to check user-declared parent dependencies too. Failing to do so leads to a crash and the user is served with a mysterious stack trace.

@nyalldawson
Copy link
Collaborator

What do you think about adding a unit test for this? We should start building up the test coverage of processing non-algorithm code. #3739 has the start of a unit test file for modeller.

@nirvn
Copy link
Contributor Author

nirvn commented Nov 10, 2016

@nyalldawson I'm not against the idea :) processing would definitively benefit from increased unit tests.

Feel free to merge this PR now, I'll create a PR for a test unit when you merge your PR.

@nirvn
Copy link
Contributor Author

nirvn commented Nov 10, 2016

@nyalldawson , one set of unit tests could be built around loading a test.model file which includes a bunch of features to look over (parent alg being one).

@nyalldawson
Copy link
Collaborator

Ok while #3739 gets reviewed I've pushed part of the test as 10648df

This should allow you to add the test covering this part.

@nirvn
Copy link
Contributor Author

nirvn commented Nov 10, 2016

Excellent.

@nirvn nirvn force-pushed the processing_fix_hasdependencies branch 3 times, most recently from 32b211e to 45b2cb5 Compare November 11, 2016 05:22
@nirvn
Copy link
Contributor Author

nirvn commented Nov 11, 2016

@nyalldawson , test covering parent alg dependency added.

@@ -66,6 +66,15 @@ def testModelerParametersDialogAvailableValuesOfType(self):
self.assertEqual(set(p.name for p in dlg.getAvailableValuesOfType([ParameterString, ParameterNumber, ParameterFile])),
set(['string', 'string2', 'number', 'file']))

a = Algorithm("qgis:clip")
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be in it's own function, not at the end of testModelerParametersDialogAvailableValuesOfType ;)

@nirvn nirvn force-pushed the processing_fix_hasdependencies branch from 45b2cb5 to be2223f Compare November 11, 2016 06:08
@nirvn
Copy link
Contributor Author

nirvn commented Nov 11, 2016

@nyalldawson , oups; I've now moved the test into its own function, and added a few lines to test algorithm output dependency too.

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.

None yet

2 participants