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: run query engine tests with query compiler #5146

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

aqrln
Copy link
Member

@aqrln aqrln commented Feb 3, 2025

@aqrln aqrln added this to the 6.4.0 milestone Feb 3, 2025
Copy link

codspeed-hq bot commented Feb 3, 2025

CodSpeed Performance Report

Merging #5146 will not alter performance

Comparing push-qrnvpsponvyr (1dfe965) with main (b06b231)

Summary

✅ 11 untouched benchmarks

Copy link
Contributor

github-actions bot commented Feb 3, 2025

WASM Query Engine file Size

Engine This PR Base branch Diff
Postgres 2.121MiB 2.121MiB 0.000B
Postgres (gzip) 849.195KiB 849.191KiB 4.000B
Mysql 2.083MiB 2.083MiB 0.000B
Mysql (gzip) 834.658KiB 834.656KiB 2.000B
Sqlite 1.992MiB 1.992MiB 0.000B
Sqlite (gzip) 798.486KiB 798.484KiB 2.000B

@aqrln aqrln force-pushed the push-qrnvpsponvyr branch 16 times, most recently from 22900bc to f9fd55e Compare February 5, 2025 11:30
@aqrln aqrln changed the base branch from main to push-qoyxpstyypys February 5, 2025 11:33
@aqrln aqrln force-pushed the push-qrnvpsponvyr branch from f9fd55e to a41edef Compare February 5, 2025 11:35
@aqrln aqrln force-pushed the push-qoyxpstyypys branch from d157164 to c468ec2 Compare February 5, 2025 11:35
@aqrln aqrln force-pushed the push-qrnvpsponvyr branch from a41edef to e212505 Compare February 5, 2025 13:15
@aqrln aqrln force-pushed the push-qoyxpstyypys branch from c468ec2 to eaefcbd Compare February 5, 2025 13:15
@aqrln aqrln force-pushed the push-qrnvpsponvyr branch from e212505 to 3f14267 Compare February 5, 2025 13:52
@aqrln aqrln force-pushed the push-qoyxpstyypys branch from eaefcbd to ffebe63 Compare February 5, 2025 13:52
@aqrln aqrln force-pushed the push-qrnvpsponvyr branch 2 times, most recently from 0f58c9e to c462b32 Compare February 5, 2025 14:54
Base automatically changed from push-qoyxpstyypys to main February 5, 2025 15:52
@aqrln aqrln force-pushed the push-qrnvpsponvyr branch from c462b32 to e0162d2 Compare February 5, 2025 15:55
@aqrln aqrln force-pushed the push-qrnvpsponvyr branch 13 times, most recently from 6251de8 to 5d29f7f Compare February 10, 2025 17:33
@aqrln aqrln marked this pull request as ready for review February 10, 2025 17:34
@aqrln aqrln requested a review from a team as a code owner February 10, 2025 17:34
@aqrln aqrln requested review from FGoessler and removed request for a team February 10, 2025 17:34
@aqrln aqrln force-pushed the push-qrnvpsponvyr branch 2 times, most recently from 6847763 to 17710bb Compare February 10, 2025 19:32
type: 'teardown',
})

await schemaState.worker.terminate()
Copy link
Member Author

Choose a reason for hiding this comment

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

put it in finally

.await
.unwrap();
if let Err(e) = teardown_project(&datamodel, Default::default(), runner.schema_id()).await {
if !expected_to_fail {
Copy link
Member Author

Choose a reason for hiding this comment

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

log the error either way

Ok::<(), TestError>(())
}
Err(_) => {
eprintln!("test panicked as expected");
Copy link
Member Author

Choose a reason for hiding this comment

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

downcast the panic and print the message

@aqrln aqrln force-pushed the push-qrnvpsponvyr branch 2 times, most recently from 62973b5 to e7d5972 Compare February 11, 2025 12:02
@aqrln aqrln force-pushed the push-qrnvpsponvyr branch from e7d5972 to 1dfe965 Compare February 12, 2025 17:04
Comment on lines +380 to +381
let last = parts.len() - 1;
parts[last] = original_test_function_name;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
let last = parts.len() - 1;
parts[last] = original_test_function_name;
parts.pop();
parts.push(original_test_function_name);

I like to avoid index arithmetic when possible

Comment on lines +8 to +9
static IGNORED_TESTS: OnceLock<Vec<String>> = OnceLock::new();
static SHOULD_FAIL_TESTS: OnceLock<Vec<String>> = OnceLock::new();
Copy link
Contributor

Choose a reason for hiding this comment

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

why not HashSet

@@ -60,6 +60,9 @@ build() {
local CONNECTOR="$1"
local CARGO_TARGET_DIR
CARGO_TARGET_DIR=$(cargo metadata --format-version 1 | jq -r .target_directory)

export RUSTFLAGS="-C link-args=-zstack-size=16777216"
Copy link
Contributor

Choose a reason for hiding this comment

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

might be worth leaving a comment why we set the stack size

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