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

Add new change types for properties on subclassing #830

Merged
merged 12 commits into from May 17, 2019

Conversation

Projects
None yet
2 participants
@franktominc
Copy link
Contributor

commented Apr 22, 2019

No description provided.

@franktominc

This comment has been minimized.

Copy link
Contributor Author

commented Apr 22, 2019

Moved from #825

@franktominc

This comment has been minimized.

Copy link
Contributor Author

commented Apr 22, 2019

There are some failing tests here that I'm not sure how to fix. It seems to be using ListChangeAppender to calculate the changes between a list and another class instance. It tries to convert the class instance to a list and fails with a ClassCastException.

The tests in question are:

org.javers.repository.mongo.EmbeddedMongoE2EWithRandomGeneratorTest/org.javers.core.JaversRepositoryE2ETest#should allow for property type change

org.javers.repository.mongo.EmbeddedMongoE2ETest/org.javers.core.JaversRepositoryE2ETest#should allow for property type change

Can you help me with those?

franktominc added some commits Apr 22, 2019

@bartoszwalacik

This comment has been minimized.

Copy link
Member

commented Apr 24, 2019

huge PR :) give me a while

@franktominc

This comment has been minimized.

Copy link
Contributor Author

commented Apr 25, 2019

No worries, take your time :)

@bartoszwalacik

This comment has been minimized.

Copy link
Member

commented Apr 29, 2019

there are two problems with this PR

  • You didn't provided tests for the new feature, so we don't know how it works and if it works.
  • You have changed some existing tests which should not be affected by this change. For example, there is no reason to change SnapshotDifferIntegrationTest.groovy or JaversNapCategoryTreeIntegrationTest.groovy. The more tests you change, the higher risk that you break existing code of Javers users
@franktominc

This comment has been minimized.

Copy link
Contributor Author

commented May 1, 2019

Regarding your points:

  • I'll write some tests for it, however I'm afraid my groovy isn't that sharp, I'll try my best.
  • The problem with the tests you mentioned was that they were testing for class equality, and this PR indeed broke some things. Now both ValueChange and ReferenceChange are abstract classes and each one of them has a final class for the added, updated and removed change.

Before the change:

Imgur

After the change:

Imgur

I updated those tests to reflect this change.

franktominc added some commits May 2, 2019

bartoszwalacik added a commit that referenced this pull request May 9, 2019

bartoszwalacik added a commit that referenced this pull request May 10, 2019

bartoszwalacik added a commit that referenced this pull request May 10, 2019

bartoszwalacik added a commit that referenced this pull request May 10, 2019

@bartoszwalacik bartoszwalacik merged commit 8d694c0 into javers:master May 17, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.