Skip to content
This repository has been archived by the owner on Jan 10, 2022. It is now read-only.

[TG-5859] Always use Objenesis to instantiate objects #59

Merged
merged 2 commits into from
Dec 19, 2018

Conversation

antlechner
Copy link
Contributor

We previously used real constructors if a constructor with no arguments was defined in the class to instantiate. Such a constructor could have code with side effects that Diffblue Cover is not analysing, which could lead to failing tests, see the regression test in this PR (added to a new folder regression to distinguish it from the unit tests generated by Diffblue Cover).
It makes sense to always use Objenesis to create objects so we don't have to worry about accidentally causing side effects anywhere when we use reflection.

@antlechner
Copy link
Contributor Author

It looks like the Travis command to install openjdk6 is failing. This also happens on develop.

@antlechner antlechner force-pushed the antonia/objenesis-everywhere branch 2 times, most recently from c8d49d0 to ce16df8 Compare December 14, 2018 15:47
@antlechner antlechner changed the base branch from develop to antonia/pom-java6-compatibility December 14, 2018 15:47
@antlechner
Copy link
Contributor Author

Based on #60, which has to be merged first as Travis no longer supports openjdk6.

Copy link

@allredj allredj left a comment

Choose a reason for hiding this comment

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

Seems sensible

@antlechner antlechner force-pushed the antonia/pom-java6-compatibility branch from 50ae2cd to 5b822ae Compare December 17, 2018 15:39
@antlechner antlechner force-pushed the antonia/objenesis-everywhere branch 2 times, most recently from 6d2a9e7 to 7d2ba28 Compare December 17, 2018 18:39
@antlechner antlechner changed the base branch from antonia/pom-java6-compatibility to develop December 17, 2018 18:41
Copy link
Contributor

@thk123 thk123 left a comment

Choose a reason for hiding this comment

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

Docs need updating but thanks for adding a test!

src/main/java/com/diffblue/deeptestutils/Reflector.java Outdated Show resolved Hide resolved
@@ -0,0 +1,12 @@
package com.diffblue.deeptestutils.regression;
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 It would be super awesome if you were you to add tests for what happens when creating abstract classes and interfaces since that code is a complicated under tested mess

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will do this in the follow-up PR for https://diffblue.atlassian.net/browse/TG-5895, as that will fix the exception wrapping for interfaces and abstract classes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tests added in #61, with an additional test enabled in #62.

Calling real constructors is problematic as they might have side effects
that are not considered by Diffblue Cover.
For example, a field could be set by calling a method on a static field,
and Diffblue Cover would not know that this field should be set to
something non-null. See the following commit for an example.
This test would fail without the changes from the previous commit.
Copy link
Contributor

@thk123 thk123 left a comment

Choose a reason for hiding this comment

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

Got to love it when you start picking at a bit of code a find a nest of bugs...

@antlechner antlechner merged commit 96d1abe into develop Dec 19, 2018
@antlechner antlechner deleted the antonia/objenesis-everywhere branch December 19, 2018 15:24
@antlechner antlechner changed the title Always use Objenesis to instantiate objects [TG-5859] Always use Objenesis to instantiate objects Jan 11, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants