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

core/state: deleted field in StateObject Copy() and unit test #1781

Merged
merged 2 commits into from
Sep 9, 2015

Conversation

Gustav-Simonsson
Copy link

No description provided.

@robotally
Copy link

Vote Count Reviewers
👍 2 @fjl @obscuren
👎 0

Updated: Wed Sep 9 16:41:39 UTC 2015

so0Restored := state.GetStateObject(stateobjaddr0)
so1Restored := state.GetStateObject(stateobjaddr1)
// non-deleted is equal (restored)
compareStateObjects(so0, so0Restored, t)
Copy link
Contributor

Choose a reason for hiding this comment

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

if !reflect.DeepEqual(so0, so0Restored) { ... } could also work.

Copy link
Author

Choose a reason for hiding this comment

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

Right, though then it could be harder to see what's wrong given that these objects have quite a few fields. Especially storage fields would be tricky to see if just printing a deep version of the objects.

Copy link
Contributor

Choose a reason for hiding this comment

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

There is always spew.Dump.

Copy link
Author

Choose a reason for hiding this comment

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

Tried that but it prints 61 lines per state object which makes comparison a bit harder, especially if the test would later fail and one checks CI output.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd personally prefer that to maintaining a 43-line function. But this is not a critical issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

There's a fatal flaw to this approach; adding new attributes will not be caught by this check. I'm adding an additional DeepEqual that tests the test and should fail if new flags have been added (and obviously not added to test)

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually DeepEqual doesn't work due to pointers. I'm set on fixing this however

@obscuren
Copy link
Contributor

obscuren commented Sep 9, 2015

👍

@fjl
Copy link
Contributor

fjl commented Sep 9, 2015

Having stared at it for 30min, I cannot think of anything this could break. 👍

fjl added a commit that referenced this pull request Sep 9, 2015
core/state: deleted field in StateObject Copy() and unit test
@fjl fjl merged commit 90f1fe0 into ethereum:develop Sep 9, 2015
@fjl fjl removed the review label Sep 9, 2015
fjl added a commit that referenced this pull request Sep 9, 2015
core/state: deleted field in StateObject Copy() and unit test
(cherry picked from commit 90f1fe0)
fjl added a commit that referenced this pull request Sep 9, 2015
core/state: deleted field in StateObject Copy() and unit test
(cherry picked from commit 90f1fe0)
Gustav-Simonsson pushed a commit that referenced this pull request Sep 10, 2015
* origin/release/1.1.0:
  cmd/geth: bump 1.1.3
  Merge pull request #1781 from Gustav-Simonsson/state_object_copy
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.

5 participants