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

Avoid test cases that test Spring Data Commons' internals [DATAGEODE-291] #336

Closed
spring-projects-issues opened this issue Jan 16, 2020 · 1 comment

Comments

@spring-projects-issues
Copy link

@spring-projects-issues spring-projects-issues commented Jan 16, 2020

Oliver Drotbohm opened DATAGEODE-291 and commented

Some methods in MappingPdxSerializerUnitTests actually test the behavior of classes in Spring Data Commons. Especially around EntityInstantiators API. Those tests now break as they make (now changed) assumptions about the way that the Commons APIs behave. Let's remove those tests for now


Referenced from: commits 3e45bf6, 4139d17, a395950, 11058af

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Jan 17, 2020

John Blum commented

I am not a fan of removing tests!  Especially when you consider the tests involved and the importance of the MappingPdxSerializer doing the right thing!

 

As such, I have:

 

  1. Reintroduced (reinstated) these [tests|https://github.com/spring-projects/spring-data-geode/blob/master/spring-data-geode/src/test/java/org/springframework/data/gemfire/mapping/MappingPdxSerializerIntegrationTests.java#L168-L214], this time in the MappingPdxSerializerIntegrationTests class as proper "Integration Tests" given the nature of these tests.

 

  1. I also introduced proper [Unit Tests|https://github.com/spring-projects/spring-data-geode/blob/master/spring-data-geode/src/test/java/org/springframework/data/gemfire/mapping/MappingPdxSerializerUnitTests.java#L469-L508] to cover the functional/behavioral contract of the MappingPdxSerializer class w.r.t. its dependency on the SD Commons API and classes in question.

 

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

Successfully merging a pull request may close this issue.

None yet
2 participants