Skip to content

Conversation

stepansergeevitch
Copy link
Collaborator

  • refactored out common unit tests into common module
  • added unit and integration tests for inf and nan values

@stepansergeevitch stepansergeevitch self-assigned this Feb 8, 2024
@stepansergeevitch stepansergeevitch requested a review from a team as a code owner February 8, 2024 12:34
@stepansergeevitch stepansergeevitch force-pushed the FIR-29921-add-inf-and-nan-tests-to-python-sdk branch from ab30825 to 9c28972 Compare February 8, 2024 12:35
), "Invalid bytea data returned after roundtrip"


@fixture(scope="session")
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason to change this? It's scoped to session if we need to reuse the database, but it's not autoused so only relevant tests will invoke it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This fixture failed for me since it's using connection_system_engine_no_db, which itself is per test scoped. This causes pytest to fail. I'm not sure how it used to work and if it's related to my changes. I'll take a look

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is the error I get locally on the main now: ScopeMismatch: You tried to access the function scoped fixture connection_system_engine_no_db with a session scoped request object, involved factories:

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It works on github CI because currently the use_database test (which is the one that uses this fixture) is skipped for staging

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. That's annoying :(
My worry here is if we reuse this fixture it might run into race condition when one test is trying to delete a db and another one to use it. Maybe let's add a few random numbers just to be safe?

@stepansergeevitch
Copy link
Collaborator Author

@stepansergeevitch stepansergeevitch force-pushed the FIR-29921-add-inf-and-nan-tests-to-python-sdk branch from 67ab6cf to 0c926b2 Compare February 12, 2024 09:53
Copy link
Contributor

@ptiurin ptiurin left a comment

Choose a reason for hiding this comment

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

Otherwise lgtm

), "Invalid bytea data returned after roundtrip"


@fixture(scope="session")
Copy link
Contributor

Choose a reason for hiding this comment

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

I see. That's annoying :(
My worry here is if we reuse this fixture it might run into race condition when one test is trying to delete a db and another one to use it. Maybe let's add a few random numbers just to be safe?

Copy link

Quality Gate Passed Quality Gate passed

Issues
0 New issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

@stepansergeevitch stepansergeevitch merged commit da4ca64 into main Feb 12, 2024
@stepansergeevitch stepansergeevitch deleted the FIR-29921-add-inf-and-nan-tests-to-python-sdk branch February 12, 2024 11:35
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