Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve AMQP Integration Tests #117

Merged
merged 7 commits into from Oct 9, 2017

Conversation

Nathan-Schwartz
Copy link
Member

Changes

  • Remove legacy amqp tests
  • Add important test cases for RPC
  • Enhance purge function to allow clearing queues without deleting. Clears queues between tests and deletes queues and exchanges before and after suites.
  • Fix type error in AMQP Transporter
  • Run the same test suite for built-in and AMQP balancing
  • preferLocal = false for RPC suite
  • Run AMQP integration tests serially to prevent unexpected states

@icebob let me know if you any questions or concerns 馃憤

@coveralls
Copy link

coveralls commented Oct 9, 2017

Coverage Status

Coverage remained the same at 98.543% when pulling 2a1886f on Nathan-Schwartz:amqp-integration-tests into 963e30a on ice-services:master.

@icebob
Copy link
Member

icebob commented Oct 9, 2017

Thanks Nathan, I'm checking....

- Remove old amqp tests
- Enhance purge function to allow clearing queues without deleting
- Fix type error in AMQP Transporter
- Run the same test suite for built-in and AMQP balancing
- PreferLocal = false for rpc suite.
@coveralls
Copy link

coveralls commented Oct 9, 2017

Coverage Status

Coverage remained the same at 98.543% when pulling 327bcf3 on Nathan-Schwartz:amqp-integration-tests into 963e30a on ice-services:master.

@Nathan-Schwartz
Copy link
Member Author

Just made a quick change so purge.js makes new channels instead of new connections.

@icebob
Copy link
Member

icebob commented Oct 9, 2017

@Nathan-Schwartz so, the rpc test is good, and I need to fix AMQP transporter to test will be passed?

@Nathan-Schwartz
Copy link
Member Author

Nathan-Schwartz commented Oct 9, 2017

@icebob Yes, I think so.

I would expect these to fail, currently:
screen shot 2017-10-09 at 3 16 52 pm

But sometimes the load balancing one fails, because ack happens before the action handler completes.

@icebob
Copy link
Member

icebob commented Oct 9, 2017

The last 2 test cases don't work with built-in balancer, only with disabled balancer

@Nathan-Schwartz
Copy link
Member Author

@icebob Can you clarify?

Are you saying that you don't want them to work with built-in balancer, or that it doesn't currently?

@icebob
Copy link
Member

icebob commented Oct 9, 2017

It can't work, because Moleculer balancer put the request to the (i.e.) MOL.REQ.worker-1 queue. If no ack, RabbitMQ won't move this request to the MOL.REQ.worker-2 queue.

@icebob
Copy link
Member

icebob commented Oct 9, 2017

And also availability balancing is working only disabled balancer.
Broker put the request to *.worker-1, *.worker-2, *.worker-3.
But if you disable balancer, broker will put the request to MOL.REQB.test.hello queue.

@Nathan-Schwartz
Copy link
Member Author

@icebob Ahh, okay. I didn't realize that. Thanks!

I will update the PR.

@icebob
Copy link
Member

icebob commented Oct 9, 2017

No, I fixed it :)

@icebob
Copy link
Member

icebob commented Oct 9, 2017

So, your first AMQP implementation was a disabled balancer solution, just I added the built-in balancer solution too.

@Nathan-Schwartz
Copy link
Member Author

@icebob That looks really good!

I think maybe you switched true and false in the describe blocks 馃檪

@icebob
Copy link
Member

icebob commented Oct 9, 2017

Yes, fixed :)
Btw, the test doesn't exit gracefully, maybe missing broker.stop?!?!

@brad-decker
Copy link

The collaboration on this is amazing. Thanks @Nathan-Schwartz and @icebob !

@icebob
Copy link
Member

icebob commented Oct 9, 2017

Ohh, the --runInBand blocking the exit.

@coveralls
Copy link

coveralls commented Oct 9, 2017

Coverage Status

Coverage decreased (-0.3%) to 98.276% when pulling 49507ea on Nathan-Schwartz:amqp-integration-tests into 963e30a on ice-services:master.

@icebob
Copy link
Member

icebob commented Oct 9, 2017

@Nathan-Schwartz Do you know how we can use RabbitMQ on Travis CI?

@Nathan-Schwartz
Copy link
Member Author

@icebob Ohh interesting. Nice find with forceExit 馃憤

I haven't set up RabbitMQ with Travis before, but it looks it shouldn't be too hard to configure:
https://docs.travis-ci.com/user/database-setup/#RabbitMQ

@icebob
Copy link
Member

icebob commented Oct 9, 2017

Great! I'm trying...

@coveralls
Copy link

coveralls commented Oct 9, 2017

Coverage Status

Coverage decreased (-0.3%) to 98.276% when pulling 46b148e on Nathan-Schwartz:amqp-integration-tests into 963e30a on ice-services:master.

@coveralls
Copy link

coveralls commented Oct 9, 2017

Coverage Status

Coverage decreased (-0.3%) to 98.276% when pulling 511cd04 on Nathan-Schwartz:amqp-integration-tests into 963e30a on ice-services:master.

@Nathan-Schwartz
Copy link
Member Author

馃帀

@icebob
Copy link
Member

icebob commented Oct 9, 2017

Wow, It passed! 馃帀 :)

@Nathan-Schwartz
Copy link
Member Author

Thanks Icebob!

@icebob
Copy link
Member

icebob commented Oct 9, 2017

Thanks Nathan, good job!

@icebob icebob merged commit 6d850ed into moleculerjs:master Oct 9, 2017
@icebob
Copy link
Member

icebob commented Oct 9, 2017

Now I have to go, I'm at midnight :)
I'm going to release a new version tomorrow.

@icebob
Copy link
Member

icebob commented Oct 10, 2017

I released a new version with this fix.
https://github.com/ice-services/moleculer/releases/tag/v0.11.3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants