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: Refactor integration tests #2362

Merged
merged 15 commits into from
Apr 3, 2023
Merged

test: Refactor integration tests #2362

merged 15 commits into from
Apr 3, 2023

Conversation

Jelenkee
Copy link
Contributor

@Jelenkee Jelenkee commented Apr 1, 2023

@Jelenkee Jelenkee marked this pull request as draft April 1, 2023 17:15
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.

Awesome, thanks a lot @Jelenkee — this is shaping up to be really excellent code. I left a few comments. Happy to merge now and do those changes in another PR — as you wish.

prql-compiler/tests/integration/README.md Outdated Show resolved Hide resolved
prql-compiler/Cargo.toml Show resolved Hide resolved
prql-compiler/tests/integration/main.rs Outdated Show resolved Hide resolved
prql-compiler/tests/integration/main.rs Outdated Show resolved Hide resolved
prql-compiler/tests/integration/main.rs Outdated Show resolved Hide resolved
.github/workflows/test-rust.yaml Outdated Show resolved Hide resolved
"{} == {}: {test_name}",
first_result.0,
k
);
}
Copy link
Member

Choose a reason for hiding this comment

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

Your implementation is probably better at debugging, so feel completely free to keep it.

That said, I think it's possible to simplify it a bit — here's one approach. The result is basically the same (the only difference is that the snapshot file contains the input prql as the expression, because of the third item passed to assert_snapshot!)

    #[test]
    fn test_rdbms() {
        let runtime = Runtime::new().unwrap();
        let mut connections = get_connections(&runtime);

        for con in &mut connections {
            setup_connection(con.as_mut(), &runtime);
        }

        // for each of the queries
        glob!("queries/**/*.prql", |path| {
            // read
            let prql = fs::read_to_string(path).unwrap();

            if prql.contains("skip_test") {
                return;
            }

            for con in &mut connections {
                let vendor = con.get_dialect().to_string().to_lowercase();
                if prql.contains(format!("skip_{}", vendor).as_str()) {
                    continue;
                }
                let result = run_query(con.as_mut(), prql.as_str(), &runtime);
                let result_string = result.iter().map(|row| row.join(",")).join("\n");

                assert_snapshot!("rdbms".to_string(), &result_string, &prql);
            }
        });
    }

(hopefully this is helpful feedback, rather than dispiriting that someone is "rewriting your code", let me know either way)

expression: result_string
input_file: prql-compiler/tests/integration/queries/invoice_totals.prql
---
2009-01,2009-01-01,1,2,1.98,2,
Copy link
Member

Choose a reason for hiding this comment

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

Not on you at all, and def not necessary in this PR, but one thing we could do is adjust the queries so they produce more aggregated results — we probably get the same test coverage with more legible outputs...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure.
And we have to remove the skips for certain dbs. Especially MsSQL is hardly tested.

Jelenkee and others added 2 commits April 2, 2023 21:38
Co-authored-by: Maximilian Roos <5635139+max-sixty@users.noreply.github.com>
@max-sixty
Copy link
Member

Looking great! We can merge whenever you're ready!

max-sixty added a commit to max-sixty/prql that referenced this pull request Apr 2, 2023
Taking my own advice from PRQL#2362 — using the tertiary syntax can reduce the number of steps

Merge after PRQL#2362, as it'll have some conflicts.
@Jelenkee
Copy link
Contributor Author

Jelenkee commented Apr 3, 2023

@max-sixty Docs have been added. You may have a look at it. If everything is fine, you can merge

Co-authored-by: eitsupi <50911393+eitsupi@users.noreply.github.com>
@max-sixty
Copy link
Member

Great — am promoting it from Draft and will merge!

@max-sixty max-sixty marked this pull request as ready for review April 3, 2023 17:01
@max-sixty max-sixty enabled auto-merge (squash) April 3, 2023 17:02
@max-sixty max-sixty merged commit f4eb8c7 into PRQL:main Apr 3, 2023
max-sixty added a commit that referenced this pull request Apr 3, 2023
Taking my own advice from #2362 — using the tertiary syntax can reduce
the number of steps

Merge after #2362, as it'll have some conflicts.
@max-sixty
Copy link
Member

Thanks @Jelenkee !

Is there anything else remaining on the infra? I think it's in a very good state.

If you thought #2362 (comment) was valid, fine to make that change.

We could also start thinking about the current exclusions — was there anything you saw that's a PRQL bug (more than just the DB doesn't support it — in which case maybe we could improve the error messages but can't make it work...)?

@Jelenkee Jelenkee deleted the refactor-integration-tests branch April 13, 2023 18:58
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