Skip to content
This repository has been archived by the owner on May 9, 2019. It is now read-only.

Using ctx.commandFailed with a subclass of TransportException breaks test #108

Open
yg-apaza opened this issue Jun 23, 2017 · 6 comments
Open
Labels
maintenance Upgrading Lagom potentially caused the issue. status:backlog

Comments

@yg-apaza
Copy link
Contributor

Current state:

  • AuctionEntity
  • PItemEntity
    • Use ctx.commandFailed with UpdateFailureException which isn't a subclass of TransportException and implements equals and hashCode
      Test is also passing
    • Use ctx.commandFailed with NotFound which is a subclass of TransportException
      Test is ignored
  • TransactionEntity

We need to tidy up this, whether throwing an exception or using ctx.commandFailed in all services

@yg-apaza yg-apaza changed the title Using ctx.commandFailed with a subclass of TransportException breaks test Using ctx.commandFailed with a subclass of TransportException breaks test Jun 23, 2017
@ignasi35
Copy link
Contributor

ignasi35 commented Jun 23, 2017

Hmmm, this is interesting.
It looks like the TestDriver doesn't capture the exceptions when thrown (instead of ctx.failed) and so the exception is not serialized/deserialized. That would explain why tests in AuctionEntity and TransactionEntity pass even though the exceptions used in those tests not alway simplement equals and hashCode.

This could be an issue. IIRC in actual core code throwing or using ctx.commandFailed on a remote singleton (PersistentEntity) is captured, serialized and sent back to the invocation point. The driver.issues() method asserts all messages are serializable but may not be asserting the serializability of the exceptions thrown. 🤔

Oh! Or maybe the common denominator is throwing an exception on a readOnlyCommandhandler. When no events are emitted (even if a response is returned), the PersistentEntityTestDriver doesn't verify serialiazablity of messages (responses or exceptions).

@TimMoore
Copy link
Contributor

Should we raise an issue in lagom/lagom about this?

@ignasi35
Copy link
Contributor

ignasi35 commented Jun 26, 2017

Should we raise an issue in lagom/lagom about this?

I'd like a double-check with this set of tests to make sure my suspicion is correct. We can raise the issue already and link it back here, I think the four tests listed by @yg-apaza are a great starting point to track and solve the issue.

@TimMoore
Copy link
Contributor

Is this the same issue? lagom/lagom#783

The creator closed it, but it sounds like he only found a workaround and it is still an actual bug.

@ignasi35
Copy link
Contributor

Is this the same issue? lagom/lagom#783

It's not the same issue. lagom/lagom#783 is about return types in a scaladsl service call while this is about serialization checks of arguments, exceptions and returned values in a persistent entity test.

@ignasi35 ignasi35 added the maintenance Upgrading Lagom potentially caused the issue. label Mar 23, 2018
@ignasi35
Copy link
Contributor

There's an issue now: lagom/lagom#1279

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
maintenance Upgrading Lagom potentially caused the issue. status:backlog
Development

No branches or pull requests

4 participants