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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

1615 invoice part 5 #1693

Merged

Conversation

xsalefter
Copy link
Contributor

@xsalefter xsalefter commented May 17, 2022

Last PR for killbill-invoice. Most of work is Guava to stream, except adding BiMap and its SimpleHashBiMap implementation. Decided to implements simple implementation because forking google BiMap implementation require a lot of Guava's classes to added as well.

I think what I've done cover all basic requirement of BiMap, there's test case for it, and also I have gist for comparing with Guava's version.

@xsalefter
Copy link
Contributor Author

@pierre @sbrossie I got the same problem as previous PR here.

@pierre
Copy link
Member

pierre commented May 17, 2022

I've restarted the job. Could you create a GitHub issue with the details, so that we don't forget to investigate the source of the flakiness?

@xsalefter
Copy link
Contributor Author

@pierre this is the new issue about this.

Copy link
Member

@sbrossie sbrossie left a comment

Choose a reason for hiding this comment

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

It looks like BiMap and its new implementation SimpleHashBiMap is only referenced once from the MockInvoiceDao - is this accurate? The MockInvoiceDao probably manages very few entries for a couple of tests and so do we really need this additional complexity, i/e couldn't we we just have the MockInvoiceDao reverse the map when needed ?

… value

See this comment: killbill#1693 (review) The method to find key by value is getAccountIdByRecordId()
@xsalefter
Copy link
Contributor Author

@sbrossie yeah I think I agree with you. I just delete BiMap and its implementation, and use internal private method to find map key by its value.

@xsalefter
Copy link
Contributor Author

@sbrossie @pierre Same problem as described in #1694 . I added new comment about my observation.

TestEntitlement#testCreateSubscriptionBillingInThePast() will assert subscription's attributes except for chargedThroughDate. See also comment in TestEntitlement#verifyChargedThroughDate()
@sbrossie sbrossie merged commit 1330756 into killbill:work-for-release-0.23.x May 18, 2022
@xsalefter xsalefter deleted the 1615-invoice_part_5 branch May 25, 2022 00:25
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

3 participants