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

Migrate tests to access configuration through public interface #457

Merged
merged 21 commits into from
May 8, 2024

Conversation

plcplc
Copy link
Contributor

@plcplc plcplc commented May 7, 2024

What

This PR ensures that all test code that relies on NDC configurations does so via the public interface rather than by directly depending on a specific configuration version format module.

The tests that have the version format as their subject matter have been moved into the configuration crate, cohabiting with the code they test.

The tests that don't have the version format as their subject matter now rely only on the public interface of the configuration crate. The biggest consequence of this by diff volume is that what used to be tables.json holding a Metadata value now becomes full configuration.json files, with their original content in the metadata section and a version: "3" tag as well.

Base automatically changed from plc/issues/PG-96 to main May 7, 2024 23:24
@plcplc plcplc marked this pull request as ready for review May 8, 2024 11:54
@plcplc plcplc requested a review from soupi May 8, 2024 12:29
@plcplc plcplc enabled auto-merge May 8, 2024 12:59
Copy link
Contributor

@soupi soupi 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 to me. One comment about a test :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Why were these removed? Do we not run them anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, because I was moving tests from the database-tests crate (I think) to the configuration crate their snapshot files would move too.

What I did was simply remove every snapshot file in the repo, re-run the tests and record new snapshots. When the dust had settled these two files were gone.

And then I remembered vaguely that I had disabled some tests of composite columns in cockroach previously because it turned out they were misconfigured somehow and configuring them correctly revealed that cockroach didn't support their use of composite types after all. So I figured these were just leftovers from that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes:

    #[ignore = "Cockroach v23.2 does not introspect composite types."]
    #[tokio::test]
    async fn select_composite_column_simple() {
        let result = run_query(create_router().await, "select_composite_column_simple").await;
        insta::assert_json_snapshot!(result);
    }

@plcplc plcplc added this pull request to the merge queue May 8, 2024
@soupi soupi removed this pull request from the merge queue due to a manual request May 8, 2024
@plcplc plcplc added this pull request to the merge queue May 8, 2024
Merged via the queue into main with commit b028131 May 8, 2024
35 checks passed
@plcplc plcplc deleted the plc/issues/PG-94 branch May 8, 2024 15:52
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