-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
[Server 3.0] Resurrect PyQgsServerWFST
- Loading branch information
Showing
1 changed file
with
1 addition
and
1 deletion.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
89d4998
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI, the test was disabled because it seemed to produce unreliable results: 6214dc1
If that's fixed, that's good news :)
89d4998
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, sorry, I thought it was removed when the switch to qt5/Py3 was made.
But generally speaking, I believe that we should think twice before removing a test and if it is really necessary, it should be done with a bit more respect for the test author and maybe saying it loud on the mailing list.
You must know very well that writing tests takes time and it's not the most exciting activity, the
rm -rf
(or blacklist either) approach in this case is not the best way to proceed in order to motivate volunteers to write and maintain tests.In general, @dakcarto also agreed that we should remove the older server scripts that is much more complex to maintain in favour of the new one which is completely python based.
If the new server script is not reliable maybe we can find a way to improve its reliability, I wasn't aware of any problem with that.
89d4998
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably have a method to blacklist tests in a different way.
When blacklisting them, they should still be executed but their success/failure noted somewhere and output logged. This way they wouldn't give false alarms on travis ci (which sometimes affect credibility of test results) but still allow for investigating the real source of troubles.
89d4998
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this was blacklisted from Travis (not removed from QGIS!) a while back. Situation was that a bunch of these tests were regularly but randomly failing and we'd see many PRs fail because of this. It was causing frustration with new contributors who were struggling to understand why their work was causing issues. It's a tough call but if Travis can't be relied upon to only flag valid errors then people start ignoring it and pushing broken code.
Re announcing it publicly - that's what the commit message was for ;) but I guess not everyone tracks master close enough to catch every commit, so in retrospect a dev list announcement should have been made.
I'll ping you next time this fails on Travis (if it's not been resolved by other changes in the meantime) for you to look into.
89d4998
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re server scripts - happy for you to make the call, but standardising on one would be much appreciated! Can we move it from port 8081 to something less commonly used, or better yet to have it auto select a free port?
89d4998
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nyalldawson yes, you can change the port by setting an env var: https://github.com/qgis/QGIS/blob/master/tests/src/python/qgis_wrapped_server.py#L31
Do you think that the problem is caused from the different concurrent builds? If that is the case, maybe there is something in the Travis env that we can use to avoid port clashing (for example a build counter and we could make port 6666 + $BUILD_COUNTER or something similar).
89d4998
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nyalldawson , I'm testing this:
so far, so good.
89d4998
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@elpaso looks good to me!
89d4998
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
implemented in #3540