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

Fix separation of columns #129

Merged
merged 7 commits into from
Oct 30, 2020
Merged

Fix separation of columns #129

merged 7 commits into from
Oct 30, 2020

Conversation

jgoizueta
Copy link

The comma-separation of columns was incorrect if the first column was skipped due to unsupported type (so a leading coman would appear: ,col1,col2.

@jgoizueta jgoizueta requested a review from Algunenano October 21, 2020 15:35
Copy link

@Algunenano Algunenano left a comment

Choose a reason for hiding this comment

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

I'm missing tests, not only on this change but also on other recent changes in the project.

@jgoizueta
Copy link
Author

jgoizueta commented Oct 23, 2020

I'm preparing a test for this case, which seems easy, but for other ongoing changes I'm missing C-level unit tests. @Algunenano any advice on how to approach them given your experience with PostGIS?

@Algunenano
Copy link

any advice on how to approach them given your experience with PostGIS?

What we do is we generate a static C library that doesn't depend on Postgresql and test that with unit tests, and then the final library is generated using that library and the files that depend on Postgres to generate the final .so that is distributed.

@jgoizueta
Copy link
Author

jgoizueta commented Oct 26, 2020

I could not add a test with the pg fdw as I intended, since there is not pg we ignore, so I had to add a test for sql server instead. Tests for sql server are currently disabled in CI, so I ran it manually and, believe me, it succeeded. I'll open a separate ticket to enable back the sql server tests using the docker I used to run the test locally, but I'll leave this PR with the disabled new test.

Copy link

@Algunenano Algunenano left a comment

Choose a reason for hiding this comment

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

Thanks for the extra effort!

@jgoizueta
Copy link
Author

jgoizueta commented Oct 29, 2020

OK, I had forgotten the disabled sql server tests runs on AppVeyor, and it has revealed a bug which wasn't observed in my sql server tests (which ran on linux). The bug, which should be fixed by e863ad1 was causing in this case the IMPORT FOREIGN SCHEMA to not import all remotes tables (the tables loop was terminated too early because the variable that controls it was being reused inside the loop.

@jgoizueta jgoizueta merged commit a9bfce5 into master Oct 30, 2020
@jgoizueta jgoizueta deleted the fix-table-columns branch October 30, 2020 07:16
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