Skip to content

Conversation

@max-hoffman
Copy link
Contributor

No description provided.

@max-hoffman max-hoffman requested a review from zachmu July 28, 2022 20:39
Copy link
Member

@zachmu zachmu left a comment

Choose a reason for hiding this comment

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

I like the sentiment of making sure that all tests pass with prepared statements, but I think this is going to cause as much confusion as it catches bugs. The thing with the magic number especially -- people are just going to change it instead of adding new prepared tests.

A better way of assuring prepared coverage would be a separate harness combined with code gen, with a manually updated list of test methods that can't be used with the prepared harness.

@max-hoffman
Copy link
Contributor Author

@zachmu If i codegen it, then I have to do it separately for memory_engine_test.go and dolt_engine_test.go, which is probably 2-3 days of automation. You don't think this is a useful stop-gap until we have a bit of time to rework testing?

@zachmu
Copy link
Member

zachmu commented Oct 11, 2022 via email

@timsehn
Copy link
Contributor

timsehn commented Mar 3, 2023

Should we just close this?

@max-hoffman
Copy link
Contributor Author

Fine w me, the checks in the Dolt repo have worked well enough on their own.

@timsehn timsehn closed this Mar 3, 2023
@Hydrocharged Hydrocharged deleted the max/count-prep-tests branch March 29, 2023 01:09
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.

4 participants