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

Update smoke test for atree inlining #348

Conversation

fxamacker
Copy link
Member

@fxamacker fxamacker commented Oct 6, 2023

Closes #343
Updates epics #292
Updates #296 #297

This PR updates smoke test to test mutation of child container:

  • existing child retrieved from parent array/map
  • new child appened/inserted to parent array
  • existing child set in parent array/map
  • new child set in parent map

This PR also added flag -slabcheck to enable in-memory slab validation and slab serialization validation. This check is time-consuming so it is disabled by default.

While at it, also refactored smoke tests.


  • Targeted PR against main branch
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work
  • Code follows the standards mentioned here
  • Updated relevant documentation
  • Re-reviewed Files changed in the Github PR explorer
  • Added appropriate labels

This commit adds flag "slabcheck" to enable checking in-memory
and serialized slabs.  This flag is off by default because it can
take a long time to run.
This commit smoke tests mutation of child container:
- existing child retrieved from parent array/map
- new child appened/inserted to parent array
- existing child set in parent array/map
- new child set in parent map
@codecov-commenter
Copy link

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (0fc8f74) 62.58% compared to head (25dbc3e) 62.58%.

Additional details and impacted files
@@                     Coverage Diff                     @@
##           feature/array-map-inlining     #348   +/-   ##
===========================================================
  Coverage                       62.58%   62.58%           
===========================================================
  Files                              15       15           
  Lines                           10559    10559           
===========================================================
  Hits                             6608     6608           
  Misses                           3008     3008           
  Partials                          943      943           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@turbolent turbolent left a comment

Choose a reason for hiding this comment

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

Great work!

I mostly focused on the Go code itself, I'm not too familiar with the original version of the stress testing

cmd/stress/array.go Outdated Show resolved Hide resolved
cmd/stress/array.go Outdated Show resolved Hide resolved
cmd/stress/array.go Outdated Show resolved Hide resolved
return
}
if nextOp == arrayMutateChildContainerAfterAppend {
index := len(expectedValues) - 1
Copy link
Member

Choose a reason for hiding this comment

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

Is there a chance that his becomes negative?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, because new element is appended to expectedValues in the same code block so expectedValues has at least one element.

atree/cmd/stress/array.go

Lines 328 to 344 in 25dbc3e

// Update expectedValues
expectedValues = append(expectedValues, expectedChildValue)
// Update array
err = array.Append(child)
if err != nil {
return nil, 0, fmt.Errorf("failed to append %s: %s", child, err)
}
if nextOp == arrayMutateChildContainerAfterAppend {
index := len(expectedValues) - 1
expectedValues[index], err = modifyContainer(expectedValues[index], child, nextNestedLevels)
if err != nil {
return nil, 0, fmt.Errorf("failed to modify child container at index %d: %w", index, err)
}
}

cmd/stress/array.go Outdated Show resolved Hide resolved
cmd/stress/array.go Outdated Show resolved Hide resolved
cmd/stress/utils.go Outdated Show resolved Hide resolved
@@ -542,3 +469,19 @@ func (s *InMemBaseStorage) SegmentsTouched() int {
func (s *InMemBaseStorage) ResetReporter() {
// not needed
}

type arrayValue []atree.Value
Copy link
Member

Choose a reason for hiding this comment

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

Nit: This looked like a single value to me at first when reading the changes above. Maybe switch the order to "value array"?

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, arrayValue is a single atree.Value, which happens to use Go array under the hood.

I added this comment to clarify:

// arrayValue is an atree.Value that represents an array of atree.Value.
// It's used to test elements of atree.Array.
type arrayValue []atree.Value

@fxamacker fxamacker changed the title Update smoke test for array/map inlining Update smoke test for atree inlining Oct 10, 2023
@fxamacker fxamacker merged commit 9b14a71 into feature/array-map-inlining Oct 10, 2023
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants