Skip to content

Conversation

@jennifersp
Copy link
Contributor

dolthub/dolt#4395 depends on this PR

Copy link
Contributor

@fulghum fulghum left a comment

Choose a reason for hiding this comment

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

Code looks great. I think this is a nice hook for GMS integrators to have.

The only suggestion I have is that it would be really cool if we could test that the new ValidateSession method is being called. We've got tests in the Dolt layer of course, but it would be awesome to have a quick tests in this layer that asserts that, too. The TestPersist test method has an example of using a SessionFactory function to create in memory sessions; that might be close to what we need. Maybe the InMemory session type could be configured with a callback function to call from the ValidateSession method that would increment a counter or something we could use just as a sanity check that the callback is being invoked correctly?

Comment on lines 22 to 25
type InMemoryBaseSession struct {
Session sql.Session
idx int
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to use InMemoryPersistedSession instead of creating/maintaining this new type?

Could we create a new constructor method, like NewInMemoryPersistedSessionWithValidationCallback that takes a function that ValidateSession could just call?

Other than that, I thought this test looked great!

Copy link
Contributor

@fulghum fulghum left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks for making sure we can test this functionality at the GMS layer, too.

@jennifersp jennifersp merged commit cd8b451 into main Sep 28, 2022
@jennifersp jennifersp deleted the jennifer/delete-branch branch September 28, 2022 17:44
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.

2 participants