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

Add a few more URLParameters tests #994

Merged
merged 1 commit into from Mar 13, 2018

Conversation

Projects
None yet
2 participants
@lukebakken
Contributor

lukebakken commented Mar 12, 2018

I tried to reproduce what was reported in #993, but couldn't, but added a few more tests.

@lukebakken lukebakken requested a review from vitaly-krugl Mar 12, 2018

self.assertEqual(parameters.virtual_host,
pika.URLParameters.DEFAULT_VIRTUAL_HOST)
def test_uses_default_virtual_host_via_encoded_slash_2(self):

This comment has been minimized.

@vitaly-krugl

vitaly-krugl Mar 13, 2018

Member

I think it may be better to use more descriptive test names instead of suffixing with 1,2,3,4. It seems like the variability is upper case vs lower case encoding and terminating slash versus no terminating slash.

@vitaly-krugl

This comment has been minimized.

Member

vitaly-krugl commented Mar 13, 2018

I tried to reproduce what was reported in #993, but couldn't

@daryasary, could you please provide a simple example that reproduces the issue that you tried to fix with #993?

@lukebakken lukebakken requested a review from vitaly-krugl Mar 13, 2018

@vitaly-krugl

This comment has been minimized.

Member

vitaly-krugl commented Mar 13, 2018

@lukebakken, could you please take a look at the software installation failures that caused a number of the build jobs to fail? This may be related to the changes in PR #984. Perhaps software installations could use some hardening or a faster source repository.

@lukebakken

This comment has been minimized.

Contributor

lukebakken commented Mar 13, 2018

It looks like Erlang Solutions' package repository is slow.

@vitaly-krugl

The test method names are good. Okay to merge once all builds pass. Please note that a number of builds failed sporadically while downloading erlang - see my comment regarding this.

@vitaly-krugl

This comment has been minimized.

Member

vitaly-krugl commented Mar 13, 2018

It looks like Erlang Solutions' package repository is slow

Is there a cache site that might be faster, more reliable?

@lukebakken

This comment has been minimized.

Contributor

lukebakken commented Mar 13, 2018

I'll try using the default Erlang package.

@lukebakken lukebakken force-pushed the lukebakken:pika-993 branch from 7cffc02 to cd78f6d Mar 13, 2018

@lukebakken

This comment has been minimized.

Contributor

lukebakken commented Mar 13, 2018

The default Erlang package for Ubuntu Trusty is too old to use (R16B03) with the latest RabbitMQ.

@lukebakken lukebakken force-pushed the lukebakken:pika-993 branch 2 times, most recently from d7da41c to 661030a Mar 13, 2018

Add a few more URLParameters tests
rename tests

Upgrade to RMQ 3.7.4, do not use ESL package

@lukebakken lukebakken force-pushed the lukebakken:pika-993 branch from 661030a to 27243ba Mar 13, 2018

@lukebakken lukebakken requested a review from vitaly-krugl Mar 13, 2018

@vitaly-krugl

Thanks! Okay to merge once all builds/tests pass. AppVeyor hasn't finished running yet.

@vitaly-krugl

Forgot to click on Approve. In any case, still need to wait for AppVeyor to pass.

@lukebakken lukebakken merged commit 16cca34 into pika:master Mar 13, 2018

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@lukebakken lukebakken deleted the lukebakken:pika-993 branch Mar 13, 2018

lukebakken added a commit that referenced this pull request Apr 13, 2018

Merge pull request #994 from lukebakken/pika-993
Add a few more URLParameters tests

(cherry picked from commit 16cca34)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment