Skip to content

Conversation

@Narigo
Copy link
Contributor

@Narigo Narigo commented Feb 8, 2016

Thanks for the patch @augeneinblick !

I've added a regexp to check that we look at a time with or without timezones and did some refactoring. Now almost all tests use vertx-unit instead of VertxTestBase - only RefCountTest is still using VertxTestBase.

There is also an additional try/catch block that handles unexpected types coming from the server.

augeneinblick and others added 6 commits February 8, 2016 08:40
* Removes VertxTestBase dependency in PostgreSQL test
* Adds better error handling
* Make queryTypeTimestampWith(out)TimezoneTest consistent to other tests

Signed-off-by: Joern Bernhardt <[email protected]>
Signed-off-by: Joern Bernhardt <[email protected]>
This was referenced Feb 8, 2016
@augeneinblick
Copy link
Contributor

Did the changes you suggested but you were faster :).
Thanks for your help and work.

@cescoffier
Copy link
Member

Looks good to me.

@pmlopes can you have a look to the date stuff ? (you're the date expert ;-)).

conn.query("SELECT * FROM test_table;", onSuccess(context, timestampSelect -> {
context.assertNotNull(timestampSelect);
context.assertNotNull(timestampSelect.getResults());
context.assertTrue(timestampSelect.getResults().get(0).getString(0).matches("\\d{4}-\\d{2}-\\d{2}T\\d{2}:\\d{2}:\\d{2}\\.\\d{3}(Z|[+-]\\d{2}:\\d{2})"));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pmlopes this would be the check I mentioned over there: 14bb98b#commitcomment-16058891

@pmlopes
Copy link
Member

pmlopes commented Feb 12, 2016

👍

pmlopes added a commit that referenced this pull request Feb 15, 2016
Fix date time and a bit refactoring
@pmlopes pmlopes merged commit f8e04ae into master Feb 15, 2016
@pmlopes pmlopes removed the to review label Feb 15, 2016
@pmlopes pmlopes added this to the 3.3.0 milestone Feb 15, 2016
@Narigo Narigo deleted the date-time-fix branch February 15, 2016 09:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

6 participants