Updated PHPUnit to version 7 + gh-24 + gh-42 + gh-85 + gh-83 + gh-71 by bigbes · Pull Request #134 · tarantool/tarantool-php

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

        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

 		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

        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

        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

 		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

 ``` 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

 	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

Conversation

bigbes commented Jul 20, 2018

Uh oh!

Totktonada left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Totktonada left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Totktonada left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Totktonada left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Totktonada commented Sep 12, 2018

Uh oh!

bigbes commented Dec 2, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants