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

fix test suite #1341

Closed
wants to merge 3 commits into from
Closed

Conversation

montanalow
Copy link
Contributor

@montanalow montanalow commented Jul 23, 2021

The test suite seems to be broken on master. I've added a bare-bones README.md with instructions for running tests, and fixes for the issues I encountered. I'm new to Rust and the ecosystem, so I'd appreciate any feedback, even minor nitpicks on things that can be handled better or more idiomatically.

  1. It doesn't appear that the default features are a compilable/testable configuration since support for a specific async runtime is now required. I've updated the suite to removed that default cargo c run, and added additional runs to cover the specific async runtimes.

  2. A MYSQL test has a deadlock because it inserts many rows with the same key. The fix is to let the key be auto-generated to avoid lock conflicts on the rows.

  3. A Postgres test for composite domain types is not supported for all versions of Postgres that are tested. The fix is to only run the test against newer versions of Postgres which support the feature.

  4. A Postgres test for the macaddr type incorrectly type casts to inet in SQL, which is invalid.

  5. docker.py references sys but doesn't import it, which results in an error.

let major_version = first.parse::<i32>().unwrap();

Ok(major_version)
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should actually be receiving this information already when establishing the connection, so we could just save this info in the connection when we receive it and then this can be a simple getter.

It'll be in ParameterStatus messages which the server sends unprompted on startup, and which we currently ignore: https://github.com/launchbadge/sqlx/blob/master/sqlx-core/src/postgres/connection/stream.rs#L107

https://www.postgresql.org/docs/13/protocol-flow.html#PROTOCOL-ASYNC

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice. It looks like this is the approach taken by @AtkinsChang in #1346. Do you want me to backport that into this PR, or wait for @AtkinsChang to clean this approach up in his rebased branch?

Copy link
Collaborator

@abonander abonander Jul 27, 2021

Choose a reason for hiding this comment

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

I had them rebase on yours already so merging that should merge both.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great. I'll consider this resolved by #1346. Also happy to open another PR after #1346 if we need more cleanup to make sure there is only one (more efficient) way to do things.

@montanalow
Copy link
Contributor Author

Closing in favor of #1346

@montanalow montanalow closed this Jul 28, 2021
@AtkinsChang
Copy link
Contributor

Sorry that I didn't find there is a duplicated PR

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