HTTP extractor #1
Conversation
tomasfejfar
force-pushed
the
tf-dev
branch
4 times, most recently
from
5a1016d to
efb4c35
Compare
composer.json
Outdated
| "@tests" | ||
| ], | ||
| "ci": [ | ||
| "@composer validate --no-check-all", |
There was a problem hiding this comment.
composer.json
Outdated
| }, | ||
| "scripts": { | ||
| "tests": "phpunit", | ||
| "phpstan": "phpstan analyse ./src --level=max --no-progress", |
composer.json
Outdated
| "phpunit/phpunit": "~6.5", | ||
| "squizlabs/php_codesniffer": "~3.2", | ||
| "phpstan/phpstan-shim": "^0.9.2", | ||
| "jakub-onderka/php-parallel-lint": "^0.9.2" |
There was a problem hiding this comment.
Chybí:
"config": {
"sort-packages": true
},
tomasfejfar
force-pushed
the
tf-dev
branch
2 times, most recently
from
9f55856 to
63e7161
Compare
phpcs.xml
Outdated
| <rule ref="SlevomatCodingStandard.Arrays.TrailingArrayComma"/> | ||
| <rule ref="SlevomatCodingStandard.Namespaces.AlphabeticallySortedUses"/> | ||
| <rule ref="SlevomatCodingStandard.Classes.ClassConstantVisibility"/> | ||
| <rule ref="SlevomatCodingStandard.TypeHints.m, "/> |
tomasfejfar
force-pushed
the
tf-dev
branch
2 times, most recently
from
8bfd689 to
cc2e654
Compare
tests/HttpExtractorTest.php
Outdated
|
|
||
| public function testExtract(): void | ||
| { | ||
| $resource = new Uri('http://example.com/result.txt'); |
There was a problem hiding this comment.
Ideálně bych to viděl v něčem, co si ten test nebo developer sám nasetupuje. Můžeš pak snadno simulovat 404, 403, 500, https, potenciálně to můžeš snadno rozšiřovat o přihlašování apod. Může to bejt něco v AWS (statický fajly v S3) nebo to může bejt třeba nějaká docker servisa s jednoduchým HTTP serverem. Tady a teď to je asi overkill, ale už ta 404 na example.com nepůjde :-)
There was a problem hiding this comment.
Ale půjde, viz nový test pod tím. Ale v rámci integračního testu to bude potřeba. To jsme se už bavili. Ale s tím budu asi potřebovat nějak pomoct.
There was a problem hiding this comment.
Ježiš, jsem si nevšiml, že ten Guzzle je mockovanej, samozřejmě, omg! 404 a vše ostatní není problém. Integrační test naladíme, to nebude problém, co všechno tam budeš chtít testovat?
|
Tam bude ve finále ten posledni integrační test
…On Jan 31, 2018 1:20 PM, "Tomáš Fejfar" ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In .travis.yml
<#1 (comment)>:
> @@ -0,0 +1,19 @@
+sudo: false
+
+services:
+ - docker
+
+before_script:
+ - docker-compose -v
+ - docker-compose build prod
+ - docker-compose run --rm prod composer ci
+
+script:
+ - docker-compose build prod
jj, myslel jsem, že musím mít něco ve "script". Hodil jsem místo toho jen
echo.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAeYC09z5nLh2f9wrVpo_gK5SUkNgaF3ks5tQFp7gaJpZM4RxDQi>
.
|
|
|
||
| before_script: | ||
| - docker -v | ||
| - docker build -t keboola/ex-http . |
There was a problem hiding this comment.
Nechal jsem si to přes noc projít hlavou a vidím tady v použití čistého dockeru ještě jeden potenciální problém. A to, že pokud jsou někde na build potřeba další servicy (třeba databáze, redis nebo tak něco), tak se to bude muset dělat buď ručně nebo přepsat zpátky na compose.
Zkusil jsem si okolo toho něco načíst, ale vypadá to, že to fakt nikdo nijak "standardizovaně" neřeší.
Hlavní problém, který řešíme je, že bysme chtěli mít jakoby víc konfigurací najednou (jednu na vývoj, jednu na travis, jednu na produkci). Mělo by jít použít víc docker-compose souborů a ty pozdějí overridují ten dřívější. Ale znamenalo by to mít v rootu jen tu defaultní "produkční" konfiguraci. A pak mít ještě šložku s travis konfigurací. A volat to docker-compose -f docker-compose.yml -f docker-compose/dev.yml up resp. docker-compose -f docker-compose.yml -f docker-compose/travis.yml up. Případně mít ten root docker-compose.yml pro vývojáře (dev) a ve složce mít extra compose pro build a produkci (tam parametr navíc nevadí - je to stejně napevno napsané v buildu).
Každopádně to zvyšuje složitost.
Nicméně pomocí tohohle a extension points by to šlo udělat DRY. Ale stále bysme se nevyhnuli více souborům :(
Je ten scénář s více službami potřebnými k testování reálný? Mohl bych projít všechny repozitáře a podívat se, ale přeci jen asi bude rychlejší, když mi to někdo kdo má přehled řekne :)
There was a problem hiding this comment.
Neni realny, vetsina komponenta potrebuje jen samu sebe, A kdyz ne, tak bych to resil az to bude aktualni, anebo proste nejak jinak - treba generic ma custom testy https://github.com/keboola/generic-extractor/blob/master/.travis.yml protoze si jeste testuje example a proxy a tech vic konfiguraci tam zadnou neplechu nedela. Asi by to slo resit pres vic docker-compose souboru, ale takhle me to prijde v pohode
There was a problem hiding this comment.
Mohl bys třeba pro mysql extraktor chtít pustit mysql server lokálně v dockeru, ale asi nevidím problém pustit docker-compose up mysql lokálně i na travisu a pak to prolinkovat s testovaným kontejnerem přes docker run --link...?
There was a problem hiding this comment.
Super, jestli jste s tím OK, tak fajn.
tomasfejfar
force-pushed
the
tf-dev
branch
5 times, most recently
from
a32b921 to
fa4f4ff
Compare
|
Prošel jsem to a za mě je to super bez výhrad. Byl bych pro merge a release. |
| use GuzzleHttp\Psr7\Uri; | ||
| use Keboola\HttpExtractor\HttpExtractor; | ||
| use PHPUnit\Framework\TestCase; | ||
| use function file_get_contents; |
There was a problem hiding this comment.
cool, I've not seen this before.
Just curious what benefit it brings and do you do that with all native methods?
| $extractor->extract($resource, $destination); | ||
| } | ||
|
|
||
| private function getMockedGuzzle(array $responses): Client |
|
OK, prošel jsem ty zbývající věci a z čeho šlo udělal samostatnou issue. Takže tohle je Ready For Merge, pokud už nikdo nic nemá. |
dohodnout- dohodli jsme se, že si každý bude používat co chce, protože to není dostatečně důležité aby na to byl sniffstaticvs$thisu assertů a podle toho upravitdohodnout a odstranit/nechat neformátovací Slevomat CS checky + pokud dávají smysl, udělat extra repository, ať se dají hlídat a upravovat na jednom místě.[PoC] Add as much of Slevomat CS as possible #2pročistit historii (?) - je otázka, jestli je to potřeba, ale přehledná moc není. Můžu to přecommitnout načisto.udělat knihovnu na konfigy a vyzkoušet ji na tomhle projektuCreate abstract config class and use it on HTTP extractor #3