You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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.
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.
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.
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?
@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).
9 commit comments
m-kuhn commentedon Sep 28, 2016
FYI, the test was disabled because it seemed to produce unreliable results: 6214dc1
If that's fixed, that's good news :)
elpaso commentedon Sep 28, 2016
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.
m-kuhn commentedon Sep 28, 2016
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.
nyalldawson commentedon Sep 28, 2016
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.
nyalldawson commentedon Sep 28, 2016
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?
elpaso commentedon Sep 28, 2016
@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).
elpaso commentedon Sep 28, 2016
@nyalldawson , I'm testing this:
so far, so good.
nyalldawson commentedon Sep 29, 2016
@elpaso looks good to me!
elpaso commentedon Sep 29, 2016
implemented in #3540