Skip to content

Conversation

@roji
Copy link
Member

@roji roji commented Sep 25, 2023

This is test cleanup work originally done in #5125 which was superceded.

@roji roji requested a review from vonzshik as a code owner September 25, 2023 22:19
Copy link
Member

@NinoFloris NinoFloris left a comment

Choose a reason for hiding this comment

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

LGTM beyond my feedback

}

[Test, Description("Roundtrips an empty multi-dimensional array.")]
public async Task Empty_multidimensional_array()
Copy link
Member

Choose a reason for hiding this comment

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

Can we add this and the zero dimension case to the test cases?

Copy link
Member Author

Choose a reason for hiding this comment

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

This one has been replaced by Write_empty_multidimensional_array, since PG normalizes empty multidimensional arrays to single-dimensional (so we can't use the generic test above with the test cases).

As for the zero dimension case, isn't it the one that's already there (test case Empty_array)?

Copy link
Member

Choose a reason for hiding this comment

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

This one has been replaced by Write_empty_multidimensional_array, since PG normalizes empty multidimensional arrays to single-dimensional (so we can't use the generic test above with the test cases).

👍

As for the zero dimension case, isn't it the one that's already there (test case Empty_array)?

Yeah Zero_dimensional seems to be a duplicate test.

Copy link
Member Author

Choose a reason for hiding this comment

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

There's no more Zero_dimensional test any more after this PR (I deleted it because the Empty_array case is the same). Is there still a duplicate test after this PR that I'm missing?

Copy link
Member

@NinoFloris NinoFloris Sep 26, 2023

Choose a reason for hiding this comment

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

Is there still a duplicate test after this PR that I'm missing?

Not at all!

@roji roji merged commit ac72fb1 into npgsql:main Sep 27, 2023
@roji roji deleted the ArrayTests branch September 27, 2023 11:15
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