Skip to content

Conversation

@robsdedude
Copy link
Contributor

No description provided.

@robsdedude robsdedude force-pushed the python-integration-test-migration branch 2 times, most recently from 2087076 to 433c185 Compare March 31, 2022 10:39
@robsdedude robsdedude force-pushed the python-integration-test-migration branch from 433c185 to 541ee4e Compare March 31, 2022 11:56
Adding all timezones the Java driver is being tested with.
Also adding a message for drivers to be able to opt out of testing of certain
timezone IDs, should their OS not support it.
@robsdedude robsdedude requested a review from fbiville April 5, 2022 14:27
self.send(req, hooks=hooks)
return self.receive(timeout, hooks=hooks)

def _check_system_support(self, type_, meta):
Copy link
Contributor

Choose a reason for hiding this comment

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

do you think it's worth introducing a general system check API now? what kind of checks do you foresee?

Copy link
Contributor Author

@robsdedude robsdedude Apr 5, 2022

Choose a reason for hiding this comment

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

Maybe ask JS if it knows the difference between 0.0 and 0 😐. I was just thinking that I don't want to introduce a message just for checking if a certain timezone is supported. That felt way to specific.

Copy link
Contributor

Choose a reason for hiding this comment

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

I usually try to generalize something once I've got evidence of several specializations but YMMV

you could argue we can start with the simple, very specific timezone check and generalize it afterwards
that being said, such a generalization, if done at a later stage, would likely be a breaking change that would affect all testkit clients

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. If it was just TestKit that's effected I wouldn't have done this. But as you say it affects the protocol and hence all backends. But I don't have a strong opinion on that. If you have, I can turn this into a specific message pair.

@@ -0,0 +1,607 @@
TZ_IDS = (
Copy link
Contributor

Choose a reason for hiding this comment

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

how are we going to maintain this over time?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't quite know. Any suggestions welcome. I mean this list should probably only ever get extended and never reduced.

@robsdedude robsdedude force-pushed the python-integration-test-migration branch from 36662e9 to b2f5a23 Compare April 7, 2022 12:14
@robsdedude robsdedude marked this pull request as ready for review April 8, 2022 14:17
@robsdedude robsdedude merged commit 0e8268a into 5.0 Apr 8, 2022
@robsdedude robsdedude deleted the python-integration-test-migration branch April 8, 2022 15:23
robsdedude added a commit to neo4j/neo4j-python-driver that referenced this pull request Apr 10, 2022
* Migrate temporal and spatial ITs to TestKit

* Introduce RegExps for skipping TestKit tests

* Add TestKit protocol messages for subtests

PR in TestKit repo:
neo4j-drivers/testkit#435
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.

3 participants