./phpunit
Details
- Reviewers
machniak - Group Reviewers
Restricted Project - Commits
- rK2cc377b6e1a6: Preliminary implementation of a greylisting database and spf service
rKef8d20e9b8cc: Preliminary implementation of a greylisting database and spf service
Diff Detail
- Repository
- rK kolab
- Branch
- arcpatch-D1810
- Lint
No Lint Coverage - Unit
No Test Coverage - Build Status
Buildable 31807 Build 12100: arc lint + arc unit
Event Timeline
src/app/Console/Commands/User/GreylistCommand.php | ||
---|---|---|
63 | In cases like this I find multiple where() calls looking better. | |
src/app/Greylist/Connect.php | ||
32 | This invokes a sql query. Checking recipient_type will be faster. | |
49 | Same as above. | |
src/app/Greylist/Request.php | ||
107 | You don't need this query if $recipient is empty, right? Move it under the if below. | |
202 | Looks like this query might be execute twice. Look above. | |
252 | Cannot these be done in a single query? | |
297 | I think these two can be replaced by ->where('updated_at', '>=', $this->timestamp->copy()->subDays(7)), with or without ->toDateTimeString(), I'm not sure. | |
304 | sizeof -> count, for consistency. | |
src/app/Http/Controllers/API/V4/PolicyController.php | ||
35 | else not needed after return. | |
73 | Calling has() and then get() looks like a redundant roundtrip. | |
135 | You don't need call_user_func_array(). | |
171 | Similar method exists in Greylist/Request. This could be de-duplicated somehow, maybe? | |
src/app/Http/Kernel.php | ||
43 | I guess we should specify different limits for the PolicyController routes. | |
src/app/User.php | ||
577 | I'm not sure I like this functionality in the User model class. | |
src/app/Utils.php | ||
267 | Utils is not that big yet, but maybe we should have NetUtils. | |
src/database/migrations/2020_10_18_091319_create_greylist_tables.php | ||
45 | The same columns like in the unique() below, so I think this is redundant. | |
50 | This also will be redundant if you re-order columns in the unique index below. | |
84 | There should be index here. | |
src/tests/Feature/Stories/GreylistTest.php | ||
48 | We have already getObjectProperty() helper. | |
src/tests/TestCaseTrait.php | ||
92 | this will be confused with existing assertEquals(), and I don't see why not just use assertSame(). | |
159 | I really don't like this extra newline. Our phpcs setup does not require this, maybe your editor? | |
325 | This will be applied to every test. I don't like it. Maybe it should be in a separate trait, that you'd add to tests you really need to. |
Some of the feedback is fixed.
src/app/Http/Controllers/API/V4/PolicyController.php | ||
---|---|---|
135 | But... call_user_func()? Or what? | |
src/app/Http/Kernel.php | ||
43 | I had not found how to actually do that. | |
src/database/migrations/2020_10_18_091319_create_greylist_tables.php | ||
45 | It is not, same as below. | |
50 | A unique constraint may cause an index to be created, but it is a different type of index -- because it can't help you speed up queries by hitting the right index in the right way. | |
src/tests/TestCaseTrait.php | ||
159 | No, my reading habits do -- I read vertical space used much, much faster than horizontal space. | |
325 | We can change it further when we have switched over most if not all tests to using volatile fixtures and not seeded contents, and split up multiple test functions per test class such that there's only one series of tests per class (currently all test functions assert the same start state). |
src/app/Http/Controllers/API/V4/PolicyController.php | ||
---|---|---|
135 | $object->senderPolicyFrameworkWhitelist($data['client_name']) ? ;) | |
src/app/Http/Kernel.php | ||
43 | I've found it could be done per route like this Route::group(['prefix' => 'api', 'middleware' => 'throttle:3,10'], function () { or probably just create a seaparate middleware label for these heavy APIs. https://laravel.com/docs/6.x/middleware#assigning-middleware-to-routes | |
src/database/migrations/2020_10_18_091319_create_greylist_tables.php | ||
45 | They are the same. $table->unique( ['sender_local', 'sender_domain', 'recipient_hash', 'net_id', 'net_type'], 'ssrnn_unq' ); | |
50 | I'll trust you know better in this case, but I always thought that unique index is used as any other index. Why not? | |
src/tests/TestCaseTrait.php | ||
325 | In my eyes this creates a mess right now. Isn't that breaking any existing tests? |
src/app/Http/Controllers/API/V4/PolicyController.php | ||
---|---|---|
135 | Of course. I complete overlooked that. | |
src/app/Http/Kernel.php | ||
43 | I reckon too that our internal infrastructure is not supposed to be limited at this level, at all. | |
src/database/migrations/2020_10_18_091319_create_greylist_tables.php | ||
50 | A unique index is a constraint that incidentally uses an index in order to speed up the lookup to pass or fail on a unique constraint. For example, a unique constraint could simply exist of a single column using a hash over cells appended to one another. It doesn't concern itself with comparison or range lookups, can completely ignore column types and is as such, when implemented correctly (so as to be most efficient as a unique constrained) in a database technology, it isn't an index you'd likely hit with a query so as to get to a resulting row. | |
src/tests/TestCaseTrait.php | ||
325 | Existing tests either use seeded content or set themselves up with a per-test domain/users/etc. In dev/testing though, all tests are intended to use one single set of 'fixtures' -- however many of those there may be, and in however many "sets" we may break them down so as to make tests run more efficiently, and to whichever extent we may indeed refactor tests such that fixtures setup and teardown can happen once per class rather than once per test. |