Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

preview_url_resource: Ellipsis must be in unicode string #1664

Merged
merged 1 commit into from Dec 3, 2016

Conversation

Projects
None yet
4 participants
Contributor

kyrias commented Dec 1, 2016

Been getting

UnicodeDecodeError: 'ascii' codec can't decode byte 0xe2 in position 0: ordinal not in range(128)

errors due to the ellipsis not being marked as a unicode string. Damn you py2.

preview_url_resource: Ellipsis must be in unicode string
Signed-off-by: Johannes Löthberg <johannes@kyriasis.com>

Can one of the admins verify this patch?

Can one of the admins verify this patch?

Can one of the admins verify this patch?

Can one of the admins verify this patch?

Can one of the admins verify this patch?

@kyrias kyrias closed this Dec 1, 2016

@kyrias kyrias deleted the kyrias:preview-url-resource-encoding branch Dec 1, 2016

@kyrias kyrias restored the kyrias:preview-url-resource-encoding branch Dec 1, 2016

Contributor

kyrias commented Dec 1, 2016

Err, whops, deleted the wrong branch .-.

@kyrias kyrias reopened this Dec 1, 2016

Can one of the admins verify this patch?

Can one of the admins verify this patch?

Can one of the admins verify this patch?

Can one of the admins verify this patch?

Can one of the admins verify this patch?

Contributor

kyrias commented Dec 1, 2016

(Also, why is it doing it five times?)

Owner

ara4n commented Dec 3, 2016

lgtm - thanks :)

@ara4n ara4n merged commit 7a00178 into matrix-org:develop Dec 3, 2016

@kyrias kyrias deleted the kyrias:preview-url-resource-encoding branch Dec 3, 2016

Member

richvdh commented Dec 5, 2016

This has broken the unit tests. Please can we not merge PRs without running the tests.

Contributor

kyrias commented Dec 5, 2016

Huh, which test? Because without it, this case is broken due to py2 handling of Unicode.

Contributor

kyrias commented Dec 5, 2016

And I just tried running the tests from master, and they succeed without any failures? Can't find any failures related to this or the develop branch after this in develop either.

Contributor

kyrias commented Dec 5, 2016

s/from master/from develop/

Member

richvdh commented Dec 5, 2016

http://matrix.org/jenkins/job/SynapseUnittestsCommit/2165/testReport/

The problems are probably in the unit tests rather than your fix, but nevertheless they need fixing. I can do so if you don't beat me to it. My snarky comment was aimed at @ara4n ...

Contributor

kyrias commented Dec 5, 2016

Ah, seems they're also using non unicode strings. Is there a simple way to run those tests from the commandline btw, since it didn't fail from setup.py test?

Member

richvdh commented Dec 5, 2016

tox -e py27 should do it

Or if you already have a virtualenv, trial tests

Member

richvdh commented Dec 5, 2016

(maybe tox doesn't work; it's a bit of a black box to me tbh)

Contributor

kyrias commented Dec 5, 2016

Yeah, it seems setup.py test runs the same tests as tox -e py27 though when I try running ./jenkins-unittests.sh I just get the help output of the coverage command, argh.

Contributor

kyrias commented Dec 5, 2016

tox -e py27 seems to run coverage properly, but no actual unit tests?

Contributor

kyrias commented Dec 5, 2016

Oh, managed to get them run by setting PYTHONPATH manually.

@kyrias kyrias referenced this pull request Dec 5, 2016

Merged

Fix preview test #1671

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment