Skip to content

Commit

Permalink
[Server 3.0] Resurrect PyQgsServerWFST
Browse files Browse the repository at this point in the history
  • Loading branch information
elpaso committed Sep 28, 2016
1 parent 49e64ab commit 89d4998
Showing 1 changed file with 1 addition and 1 deletion.
2 changes: 1 addition & 1 deletion ci/travis/linux/script.sh
Expand Up @@ -21,5 +21,5 @@ export CCACHE_TEMPDIR=/tmp

DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )"

xvfb-run ctest -V -E "qgis_openstreetmaptest|qgis_wcsprovidertest|PyQgsOfflineEditingWFS|PyQgsWFSProviderGUI|qgis_ziplayertest|$(cat ${DIR}/blacklist.txt | paste -sd '|' -)" -S ./qgis-test-travis.ctest --output-on-failure
xvfb-run ctest -V -E "qgis_openstreetmaptest|qgis_wcsprovidertest|PyQgsWFSProviderGUI|qgis_ziplayertest|$(cat ${DIR}/blacklist.txt | paste -sd '|' -)" -S ./qgis-test-travis.ctest --output-on-failure
# xvfb-run ctest -V -E "qgis_openstreetmaptest|qgis_wcsprovidertest" -S ./qgis-test-travis.ctest --output-on-failure

9 comments on commit 89d4998

@m-kuhn
Copy link
Member

@m-kuhn m-kuhn commented on 89d4998 Sep 28, 2016

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 :)

@elpaso
Copy link
Contributor Author

@elpaso elpaso commented on 89d4998 Sep 28, 2016

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.

@m-kuhn
Copy link
Member

@m-kuhn m-kuhn commented on 89d4998 Sep 28, 2016

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.

@nyalldawson
Copy link
Collaborator

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.

@nyalldawson
Copy link
Collaborator

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?

@elpaso
Copy link
Contributor Author

@elpaso elpaso commented on 89d4998 Sep 28, 2016

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).

@elpaso
Copy link
Contributor Author

@elpaso elpaso commented on 89d4998 Sep 28, 2016

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:

try:
    QGIS_SERVER_WFST_DEFAULT_PORT = os.environ['QGIS_SERVER_WFST_DEFAULT_PORT']
except:
    import socket
    s = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
    s.bind(("",0))
    QGIS_SERVER_WFST_DEFAULT_PORT = s.getsockname()[1]
    s.close()

so far, so good.

@nyalldawson
Copy link
Collaborator

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!

@elpaso
Copy link
Contributor Author

@elpaso elpaso commented on 89d4998 Sep 29, 2016

Choose a reason for hiding this comment

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

implemented in #3540

Please sign in to comment.