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 failures due to integer size differences between architectures #58

Closed
lechner opened this issue Sep 13, 2021 · 11 comments
Closed

Test failures due to integer size differences between architectures #58

lechner opened this issue Sep 13, 2021 · 11 comments

Comments

@lechner
Copy link

lechner commented Sep 13, 2021

Hi,

Your package was uploaded to the Debian archive earlier this year, where we use it for internal databases. Thank you for providing the extension!

Perhaps somewhat related to the discussion in #47, we see variances in your test suite across architectures. On both i386 and armhf, the TODO test number 31—which is expected to fail—actually succeeds.

Is there a better way to handle it than to disable the test? Thanks!

Kind regards
Felix Lechner

@theory
Copy link
Owner

theory commented Sep 19, 2021

That's weird. Here's the declaration of the struct for semver:

pg-semver/src/semver.c

Lines 59 to 64 in 87cc30c

/* memory/heap structure (not for binary marshalling) */
typedef struct semver {
int32 vl_len_; /* varlena header */
vernum numbers[3];
char prerel[]; /* pre-release, including the null byte for convenience */
} semver;

Note that it's explicitly using an int32. Compiling to make int 64 bit should not impact int32. Any idea what compilation options are impacting this?

@lechner
Copy link
Author

lechner commented Sep 19, 2021

Hi, why is that test marked TODO if it fails as expected, please? Thanks!

@theory
Copy link
Owner

theory commented Sep 19, 2021

The point of a TODO is to say "yes, I know this test fails, but want my coverage to be completely, and we will (may?) try to fix it in the future." At that time, it would not longer be a TODO.

@lechner
Copy link
Author

lechner commented Sep 19, 2021

But the test cannot be fixed. It performs as expected. You could only modify the underlying code, i.e the typedef int32 vernum;.

@theory
Copy link
Owner

theory commented Sep 19, 2021

I expect one day we will refactor semver to support larger integers, and then the test will pass. I don't know when that will be, but as this is one of the tests from the corpus, I wanted to be sure to include it for completeness.

The test should not pass. If it does, it's weird, and I'm curious to know how Postgres was compiled to make that happen.

@theory
Copy link
Owner

theory commented Sep 19, 2021

See the TAP spec for details on the meaning of TODO tests.

@lechner
Copy link
Author

lechner commented Sep 22, 2021

I'm curious to know how Postgres was compiled

You can find the logs for all architectures by clicking in the Status column here. Does that help?

@theory
Copy link
Owner

theory commented Sep 26, 2021

Nothing obvious there. Can you send me the regression.diffs file?

@theory
Copy link
Owner

theory commented Sep 26, 2021

Oh, never mind, it's in the logs you linked to, thanks.

@theory theory closed this as completed in 07b0c8b Sep 26, 2021
@theory
Copy link
Owner

theory commented Sep 26, 2021

I think 07b0c8b fixes the problem, bit of a 🤦🏻 actually. Appreciate the report!

@lechner
Copy link
Author

lechner commented Sep 28, 2021

The tests now pass on i386 and armhf. Thanks so much for the quick fix!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants