Updated PHPUnit to version 7 + gh-24 + gh-42 + gh-85 + gh-83 + gh-71 #134

bigbes · 2018-07-20T15:16:23Z

No description provided.

….1+) * Update test-run.py and lib/tarantool_server.py to support python 3 * Update tests accordingly to new version of PHPUnit * Added initialization of standart class properties to 'Tarantool'

closes tarantoolgh-24

closes tarantoolgh-42

@unix

* Added uri library from tarantool * New arg-format for connect: `tcp://..` and `..@unix\:...` URIs * Spaces -> Tabs, 2xTabs -> Tabs in tests * Use ZVAL_UNDEF instead of three separate gotos * Rewrite connection algorithm for supporting unix/tcp connection URI * Changed internal tarantool_connection table (with new struct tarantool_url) * Added new flag for test-run (--unix)

closes tarantoolgh-85

closes tarantoolgh-71

closes tarantoolgh-83

Totktonada

Comments for 'Update PHPUnit'.

Totktonada · 2018-09-10T13:22:51Z

lib/tarantool_server.py

@@ -245,7 +245,7 @@ def start(self):
        self.generate_configuration()
        if self.script:
            shutil.copy(self.script, self.script_dst)
-            os.chmod(self.script_dst, 0777)
+            os.chmod(self.script_dst, 511)


It is better to use 0o777, should work with python2 and python3 both.

Totktonada · 2018-09-10T14:17:03Z

test/MockTest.php

 {
    public function testFoo()
    {
-        $tnt = $this->getMock('Tarantool');
+        // $tnt = $this->createMock(['Tarantool']);


Should be removed?

Totktonada

Comments for 'Replace linear scan'.

Please, remove 'closes gh-24' mark, because the issue is not relevent. Moreover, it was already implemented ( 31cf8a8 ) and should be closed.

See one inline comment below.

Totktonada · 2018-09-11T00:14:09Z

src/tarantool_schema.c

-		return 0;
-	return !memcmp((*lval)->key.id, (*rval)->key.id, (*rval)->key.id_len);
-}
+#define MH_DEFINE_CMPFUNC(NAME, TYPE)								\


I trying to retain myself from formatting nitpicking, but here the entire block formatted with exceeding 80 symbols limit.

Totktonada

Comments for 'Selecting by space_no/index_name'.

Totktonada · 2018-09-11T12:44:28Z

src/tarantool.c

+	size_t body_size = php_mp_unpack_package_size(pack_len);
+	smart_string_ensure(obj->value, body_size);
+	if (tarantool_stream_read(obj, obj->value->c,
+				body_size) == FAILURE)


Underindented.

Totktonada · 2018-09-11T12:44:59Z

src/tarantool.c

+				body_size) == FAILURE)
+		return FAILURE;
+
+	struct tnt_response resp; memset(&resp, 0, sizeof(struct tnt_response));


Nit: two operators on the same line.

Totktonada · 2018-09-11T12:49:17Z

src/tarantool.c

+	obj->value->len = tp_used(obj->tps);
+	tarantool_tp_flush(obj->tps);
+
+	if (tarantool_stream_send(obj) == FAILURE)


This block (here and below) was mostly copy-pasted from get_spaceno_by_name. It worth to wrap it into a function I think.

Totktonada · 2018-09-11T13:40:22Z

src/tarantool.c

@@ -695,6 +744,11 @@ int get_indexno_by_name(tarantool_connection *obj, int space_no,
 		THROW_EXC("Index ID must be String or Long");
 		return FAILURE;
 	}
+
+	if (obtain_space_by_spaceno(obj, space_no) == FAILURE) {


I think this call should be a part of get_spaceno_by_name.

Totktonada · 2018-09-11T14:00:54Z

README.md

-$tnt = new Tarantool('localhost', 16847);
+$tnt = new Tarantool('tcp://test:test@localhost');
+$tnt = new Tarantool('tcp://test:test@localhost:3301');
+$tnt = new Tarantool('test:test@unix/:/var/tmp/tarantool.sock);


Typo: unmatched quote.

Totktonada · 2018-09-11T14:00:59Z

README.md

+$tnt = new Tarantool('tcp://test:test@localhost');
+$tnt = new Tarantool('tcp://test:test@localhost:3301');
+$tnt = new Tarantool('test:test@unix/:/var/tmp/tarantool.sock);
+$tnt = new Tarantool('unix:///var/tmp/tarantool.sock); /* if no login is needed */


Typo: unmatched quote.

Totktonada

Comments for 'Implement UNIX socket support'.

I propose to add uri.rl to track which version of the file is actually used.

See other comments below.

Totktonada · 2018-09-11T19:44:45Z

lib/tarantool_server.py

@@ -204,6 +211,7 @@ def find_exe(self):
        raise RuntimeError("Can't find server executable in " + os.environ["PATH"])

    def generate_configuration(self):
+        # print(self.args)


Forgotten debug print.

Totktonada · 2018-09-11T19:45:37Z

lib/tarantool_server.py

@@ -268,5 +284,4 @@ def clean(self):

    def __del__(self):
        self.stop()
-        self.clean()
-
+        # self.clean()


Why do you comment clean here?

Totktonada · 2018-09-11T19:50:26Z

test-run.py

-            'test/shared/tarantool-3.ini'
+            'test/shared/tarantool-1.ini'
+            # 'test/shared/tarantool-2.ini',
+            # 'test/shared/tarantool-3.ini'


What is purpose of this change? Should be commented, I think.

Totktonada · 2018-09-11T19:55:21Z

test-run.py

+    srv.start()
+
+    run_tests(srv.vardir)
+


Nit: It would be good to more or less follow PEP8 and use two empty lines to separate functions on the top level of a file.

Totktonada · 2018-09-11T20:00:04Z

test/AssertTest.php

+	static function connectTarantool() {
+		$port = getenv('PRIMARY_PORT');
+		if ($port[0] == '/') {
+			return new Tarantool('unix/:' . $port, 'prefix');


'prefix' is persistent_id value? It had no been passed before. Why do you change it?

Totktonada · 2018-09-11T22:45:09Z

src/tarantool_url.h

+tarantool_url_free(struct tarantool_url *url, int persistent);
+
+const char *
+tarantool_url_write_php_format(struct tarantool_url *url, int persistent);


It is not obvious that here we return URL w/o user and password in the format that php streams support. Need to be commented.

Totktonada · 2018-09-11T22:49:14Z

src/php_tarantool.h

-		int                      port;
-		char                    *login;
-		char                    *passwd;
+		char                    *url;


I think it wroth to comment purpose of url field, because it is not obvious considering presence of url_parsed. First poor variant of the description:

The url to perform 'physical' connection using php stream; it does not contain authentification information and uses the schema that php streams understands; see tarantool_url_write_php_format for more info.

Totktonada · 2018-09-11T22:55:40Z

src/tarantool_msgpack.c

@@ -282,10 +282,11 @@ ptrdiff_t php_mp_unpack_map(zval *oval, char **str) {
 		ZVAL_UNDEF(&key);
 		ZVAL_UNDEF(&value);
 		if (php_mp_unpack(&key, str) == FAILURE) {
-			goto error_key;
+			goto error;


Should not ZVAL_UNDEF be used here too? We assume that php_mp_unpack can write some garbage in case of a failure, yep?

Totktonada · 2018-09-11T23:24:05Z

src/tarantool_url.c

+
+struct tarantool_url *
+tarantool_url_parse(const char *url, int persistent) {
+	struct uri parsed; memset(&parsed, 0, sizeof(struct uri));


Nit: I would avoid two operators on the same line.

src/tarantool_url.c

Totktonada · 2018-09-11T23:34:35Z

README.md

@@ -114,7 +114,7 @@ _**Description**_: Available Tarantool Constants

 ``` php
 Tarantool {
-    public       Tarantool::__construct ( [ string $host = 'localhost' [, int $port = 3301 [, string $user = "guest" [, string $password = NULL [, string $persistent_id = NULL ] ] ] ] ] )
+    public       Tarantool::__construct ( [ string $uri = 'tcp://guest@localhost:3301' [, string $persistent_id = NULL ] ] )


Default port is 3303 as I see in src/tarantool_url.c.

Totktonada · 2018-09-12T00:46:48Z

src/tarantool.c

@@ -225,7 +225,7 @@ static int __tarantool_connect(tarantool_object *t_obj) {
 	TSRMLS_FETCH();
 	tarantool_connection *obj = t_obj->obj;
 	int status = SUCCESS;
-	long count = TARANTOOL_G(retry_count);
+	long count = TARANTOOL_G(retry_count) + 1;


So in case of retry_count > 0 the real retry count will be retry_count + 1? I think it should be handled in some other way.

Totktonada · 2018-09-12T00:49:01Z

The first review is done. @bigbes, the ball is on your court.

tarantool/tarantool-php#134

* Bump php version to 7.1 * Temporary skip the test for the pecl connector until this PR is merged: tarantool/tarantool-php#134 * Use variadic args in Queue::call() * Use float type hint for timestamp and interval arguments * Declare strict types * Fix scrutinizer fails to parse a coverage file * Add .php_cs.dist * Update README.md

bigbes · 2018-12-02T22:12:59Z

Superseded by #140 #141 #142 #143

bigbes added 3 commits July 1, 2018 14:19

Update PHPUnit to version 7 (old version 4.9 wasnt supported by PHP 7…

108b658

….1+) * Update test-run.py and lib/tarantool_server.py to support python 3 * Update tests accordingly to new version of PHPUnit * Added initialization of standart class properties to 'Tarantool'

Replace linear space scan with hash for update fields

e93c8ed

closes tarantoolgh-24

Selecting by space_no/index_name shouldn't throw error now

e50e288

closes tarantoolgh-42

bigbes requested a review from alg1973 July 20, 2018 15:16

bigbes changed the title ~~Updated PHPUnit to version 7 + gh-24 + gh-42~~ Updated PHPUnit to version 7 + gh-24 + gh-42 + gh-85 Aug 13, 2018

bigbes added 3 commits August 13, 2018 15:30

README.md update re UNIX socket support

bf6ef57

closes tarantoolgh-85

Do not reload schema on failed authentication

13d2653

closes tarantoolgh-71

No more assertion on 0 retries options

4567e20

closes tarantoolgh-83

bigbes changed the title ~~Updated PHPUnit to version 7 + gh-24 + gh-42 + gh-85~~ Updated PHPUnit to version 7 + gh-24 + gh-42 + gh-85 + gh-83 + gh-71 Aug 14, 2018

Use packpack instead of obsolete tarantool/build in .travis.yml

6453c4b

Totktonada reviewed Sep 10, 2018

View reviewed changes

Totktonada reviewed Sep 11, 2018

View reviewed changes

Totktonada reviewed Sep 12, 2018

View reviewed changes

rybakit added a commit to tarantool-php/queue that referenced this pull request Oct 23, 2018

Temporary skip the test for the pecl connector until this PR is merged:

6704c7e

tarantool/tarantool-php#134

First bunch of review fixes

dd8b8fe

bigbes force-pushed the php7-v2 branch 5 times, most recently from 75606c0 to 03bc225 Compare November 4, 2018 22:42

Update travis.yml

5546eae

bigbes force-pushed the php7-v2 branch from 03bc225 to 5546eae Compare November 4, 2018 22:45

bigbes closed this Dec 2, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updated PHPUnit to version 7 + gh-24 + gh-42 + gh-85 + gh-83 + gh-71 #134

Updated PHPUnit to version 7 + gh-24 + gh-42 + gh-85 + gh-83 + gh-71 #134

bigbes commented Jul 20, 2018

Totktonada left a comment

Totktonada Sep 10, 2018

Totktonada Sep 10, 2018

Totktonada left a comment

Totktonada Sep 11, 2018

Totktonada left a comment

Totktonada Sep 11, 2018

Totktonada Sep 11, 2018

Totktonada Sep 11, 2018

Totktonada Sep 11, 2018

Totktonada Sep 11, 2018

Totktonada Sep 11, 2018

Totktonada left a comment

Totktonada Sep 11, 2018

Totktonada Sep 11, 2018

Totktonada Sep 11, 2018

Totktonada Sep 11, 2018

Totktonada Sep 11, 2018

Totktonada Sep 11, 2018

Totktonada Sep 11, 2018

Totktonada Sep 11, 2018

Totktonada Sep 11, 2018

Totktonada Sep 11, 2018

Totktonada Sep 12, 2018

Totktonada commented Sep 12, 2018

bigbes commented Dec 2, 2018

Updated PHPUnit to version 7 + gh-24 + gh-42 + gh-85 + gh-83 + gh-71 #134

Updated PHPUnit to version 7 + gh-24 + gh-42 + gh-85 + gh-83 + gh-71 #134

Conversation

bigbes commented Jul 20, 2018

Totktonada left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Totktonada left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Totktonada left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Totktonada left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Totktonada commented Sep 12, 2018

bigbes commented Dec 2, 2018