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

chore: improve integration tests #3934

Merged
merged 2 commits into from
Dec 14, 2023
Merged

chore: improve integration tests #3934

merged 2 commits into from
Dec 14, 2023

Conversation

PrettyWood
Copy link
Collaborator

@PrettyWood PrettyWood commented Dec 13, 2023

Feedbacks on integration tests

When I first started to work on integration tests, I didn't know there was the great
prqlc/prql-compiler/tests/integration/dbs/README.md file that explains clearly how to run those tests.
I first had an issue by running

cargo nextest run --all-targets -F "test-dbs,test-dbs-external

and got bitten by the "it doesn't handle multiple processes"
Fortunately I saw the message

# test-dbs runs database setup when the connection is established,
# and because nextest runs test in separate processes, this happens on every test.
# To prevent mutliple setups running at once, we set max-threads to 1.

in nextest.toml and managed to run them properly

I also had issues with just starting the mssql container but this is due to my M1 and I found this issue that was already
linked in the docker-compose (microsoft/mssql-docker#668 (comment))
Thank you for that!

Once MSSQL was up and running, I thought everything would work. I was wrong 😬

The first time I run the test I got a timeout on the MSSQL server

test queries::results::aggregation has been running for over 60 seconds
.test queries::results::aggregation ... FAILED

failures:

---- queries::results::aggregation stdout ----
thread 'queries::results::aggregation' panicked at 'called `Result::unwrap()` on an `Err` value: Tls("connection closed via error")', prqlc/prql-compiler/tests/integration/dbs/protocol/mssql.rs:32:18

I also got once the error

thread 'queries::results::aggregation' panicked at 'Failed to insert row: no such table: albums.my

Caused by:
    Error code 1: SQL error or missing database

I don't remember if I stopped a test during execution or whatever but I had to dig a bit in the code
that it's just a side effect of the MySQL test that copies the csv files into xyz.my.csv.
And since they are git ignored, you don't see anything in a git status.
Anyway easy fix:

find . -type f -name '*.my.csv' -delete

So how do we fix the MSSQL timeout error?

It appears there is an issue with the TLS handshake on M1 (see prisma/tiberius#65)
I propose the switch to rustls since it doesn't require headers or runtime libraries.
(it conflicts with async_native_tls and is in the default features henece the need to deactive
those: see prisma/tiberius#317)

And when I went with this approach the handshake was done! Great so it works finally!!!
OR NOT

error[E0277]: the trait bound `BigDecimal: tiberius::FromSql<'_>` is not satisfied
   --> prqlc/prql-compiler/tests/integration/dbs/protocol/mssql.rs:64:48
    |
64  | ...                   .get::<BigDecimal, usize>(i)
    |                                          ^^^^^ the trait `tiberius::FromSql<'_>` is not implemented for `BigDecimal`
    |
    = help: the following other types implement trait `tiberius::FromSql<'a>`:
              bool
              i16
              i32
              i64
              u8
              f32
              f64
              Uuid
            and 5 others
note: required by a bound in `tiberius::Row::get`
   --> /Users/ericjolibois/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tiberius-0.12.2/src/row.rs:375:12
    |
373 |     pub fn get<'a, R, I>(&'a self, idx: I) -> Option<R>
    |            --- required by a bound in this associated function
374 |     where
375 |         R: FromSql<'a>,
    |            ^^^^^^^^^^^ required by this bound in `Row::get`

For more information about this error, try `rustc --explain E0277`.
error: could not compile `prql-compiler` (test "integration") due to previous error

Ahhhh right! By setting default-features = false, I forgot to add tds73 which is also enabled by default
Annnnnnd TADA!

Improvements

When you run a test and see

test queries::compile::math ... ok

you don't know which dialects have been tested
And with a little dbg!() macro you see that MSSQL is not tested by default!

[prqlc/prql-compiler/tests/integration/queries.rs:164] test_name = "math"
[prqlc/prql-compiler/tests/integration/queries.rs:164] dialect = DuckDb
[prqlc/prql-compiler/tests/integration/queries.rs:164] test_name = "math"
[prqlc/prql-compiler/tests/integration/queries.rs:164] dialect = Postgres
[prqlc/prql-compiler/tests/integration/queries.rs:164] test_name = "math"
[prqlc/prql-compiler/tests/integration/queries.rs:164] dialect = MySql
[prqlc/prql-compiler/tests/integration/queries.rs:164] test_name = "math"
[prqlc/prql-compiler/tests/integration/queries.rs:164] dialect = ClickHouse
[prqlc/prql-compiler/tests/integration/queries.rs:164] test_name = "math"
[prqlc/prql-compiler/tests/integration/queries.rs:164] dialect = GlareDb

So you NEED to add # mssql:test on top of the file to run it. That's weird compared to the others...
But then I looked at the code and saw that level is "UNSUPPORTED". So makes sense but again you need to know to know ;)

And the second point is just to improve/update a bit the documentation


So I would go with those changes in this PR if you're ok with it

  • make integration tests pass on M1 (for that I would like to know if it doesn't break anything for tests on Linux)
  • improve documentation

@PrettyWood PrettyWood marked this pull request as ready for review December 13, 2023 22:02
@PrettyWood
Copy link
Collaborator Author

@max-sixty Here you go 😄

Copy link
Member

@max-sixty max-sixty left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the report, and for the the PR!

@PrettyWood PrettyWood merged commit 0e8a9f9 into main Dec 14, 2023
72 checks passed
@PrettyWood PrettyWood deleted the improve-integration-tests branch December 14, 2023 07:38
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