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

test(robot-server): Test for SQL parity between metadata.create_all() and the migration path #16772

Merged
merged 4 commits into from
Nov 13, 2024

Conversation

SyntaxColoring
Copy link
Contributor

@SyntaxColoring SyntaxColoring commented Nov 12, 2024

Overview

This covers EXEC-827 with a test. The test is currently marked as expected-to-fail. See EXEC-827 for background.

Test Plan and Hands on Testing

None needed.

Review requests

Are the names and docstrings clear?

Risk assessment

No risk.

@SyntaxColoring SyntaxColoring marked this pull request as ready for review November 12, 2024 21:56
@SyntaxColoring SyntaxColoring requested a review from a team as a code owner November 12, 2024 21:56
Copy link
Member

@sfoster1 sfoster1 left a comment

Choose a reason for hiding this comment

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

Seems like the right direction to go. I'm a little confused about the intent behind all of this, though. Like, if we're going to be writing out the sql command strings anyway, why not just... write them out? It feels like we're tying ourselves in knots for the sake of a kind of mediocre statement mapper.

Copy link
Contributor

@TamarZanzouri TamarZanzouri 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! thank you for fixing this!

@SyntaxColoring SyntaxColoring merged commit 14c4bea into edge Nov 13, 2024
7 checks passed
@SyntaxColoring SyntaxColoring deleted the sql_parity_testing branch November 13, 2024 19:15
@SyntaxColoring
Copy link
Contributor Author

I'm a little confused about the intent behind all of this, though. Like, if we're going to be writing out the sql command strings anyway, why not just... write them out? It feels like we're tying ourselves in knots for the sake of a kind of mediocre statement mapper.

@sfoster1 Are you asking why use SQLAlchemy at all? Or just, why use SQLAlchemy for declaring the tables?

If the former, I think SQLAlchemy is a net positive despite its complexity. That's a bigger discussion and I'm happy to expand on it outside of this PR.

If the latter:

  • Some of our columns are more than just straight passthroughs. e.g. we have datetime and enum validation. So the Python-side table declarations do add some meaning beyond what you can express in a raw SQL string.
  • Even putting those columns aside, I'm not sure it would be possible to just write the raw SQL strings here without giving up on the rest of SQLAlchemy, just because of the way SQLAlchemy works syntactically.

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.

3 participants