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

Port Swift integration tests to pytest. #17178

Merged
merged 6 commits into from
Aug 9, 2024
Merged

Port Swift integration tests to pytest. #17178

merged 6 commits into from
Aug 9, 2024

Conversation

criemen
Copy link
Collaborator

@criemen criemen commented Aug 7, 2024

Tests pass on this PR due to a test infrastructure failure, they ought to fail.

Apparently some test databases have DB-check failures, instead of blanket-disabling db-checks for Swift, I've marked these tests now manually.

@github-actions github-actions bot added the Swift label Aug 7, 2024
@criemen criemen force-pushed the criemen/pytest-swift branch 2 times, most recently from ec4ba6a to 2e1bc46 Compare August 7, 2024 22:45
@criemen criemen added the depends on internal PR This PR should only be merged in sync with an internal Semmle PR label Aug 7, 2024
@criemen criemen force-pushed the criemen/pytest-swift branch from 2e1bc46 to e5261f8 Compare August 8, 2024 06:48
@criemen
Copy link
Collaborator Author

criemen commented Aug 8, 2024

I'm seeing the following test failure:

ql/swift/ql/integration-tests/posix-only/deduplication/test.py::test::BuiltinTypes.ql FAILED - RESULT
------------------------------ Captured diff (QL) ------------------------------
BuiltinTypes.expected <-> BuiltinTypes.actual
--- expected
+++ actual
@@ -2,7 +2,6 @@
 | Builtin.Executor | BuiltinExecutorType |
 | Builtin.FPIEEE32 | BuiltinFloatType |
 | Builtin.FPIEEE64 | BuiltinFloatType |
-| Builtin.FPIEEE80 | BuiltinFloatType |
 | Builtin.Int1 | BuiltinIntegerType |
 | Builtin.Int8 | BuiltinIntegerType |
 | Builtin.Int16 | BuiltinIntegerType |

I have no idea why this triggers on the pytest port, but not on the original branch.

@criemen criemen marked this pull request as ready for review August 8, 2024 07:24
@criemen criemen requested a review from a team as a code owner August 8, 2024 07:24
@redsun82
Copy link
Contributor

redsun82 commented Aug 8, 2024

I'm seeing the following test failure:

ql/swift/ql/integration-tests/posix-only/deduplication/test.py::test::BuiltinTypes.ql FAILED - RESULT
------------------------------ Captured diff (QL) ------------------------------
BuiltinTypes.expected <-> BuiltinTypes.actual
--- expected
+++ actual
@@ -2,7 +2,6 @@
 | Builtin.Executor | BuiltinExecutorType |
 | Builtin.FPIEEE32 | BuiltinFloatType |
 | Builtin.FPIEEE64 | BuiltinFloatType |
-| Builtin.FPIEEE80 | BuiltinFloatType |
 | Builtin.Int1 | BuiltinIntegerType |
 | Builtin.Int8 | BuiltinIntegerType |
 | Builtin.Int16 | BuiltinIntegerType |

I have no idea why this triggers on the pytest port, but not on the original branch.

I think those tests are unfortunately sensible to what runner runs them, so it might just be a question of having changed that moving to the integration test check template.

We can just accept the change, or put some effort into making the test a bit less host dependent, like skipping some of the entries in the query. Unfortunately testing Builtin stuff is inherently host-dependent...

@criemen
Copy link
Collaborator Author

criemen commented Aug 8, 2024

The query already skips Builtin.FPIEEE16, so I'd be happy to also skip Builtin.FPIEEE80 there too. I'll check if we moved from intel to arm macs here, as Builtin.FPIEEE80 sounds very much like an x86_64 thing, it'd make sense if it disappears there.

@criemen criemen merged commit 974868c into main Aug 9, 2024
18 checks passed
@criemen criemen deleted the criemen/pytest-swift branch August 9, 2024 18:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
depends on internal PR This PR should only be merged in sync with an internal Semmle PR Swift
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants