NimaSoroush / differencify

Differencify is a library for visual regression testing
MIT License
634 stars 46 forks source link

Improve Docker developer experience #84

Closed EranSch closed 6 years ago

EranSch commented 6 years ago

During work on #83, I made a few changes that I wanted to share in case they would be useful. Namely:

Thanks!

NimaSoroush commented 6 years ago

Hi @Swingline0, I was always thinking to improve this but never had a chance to do it. Thanks for putting this together

EranSch commented 6 years ago

You bet, @NimaSoroush! I suppose with the new NPM script, the contribution guidelines could also be updated. All that said, I would love if you could give this command a test on your end just to make sure it all works as expected.

NimaSoroush commented 6 years ago

I just tried that script locally. I think we still need another script to trigger the integration test only

...
    "jest:integration": "jest --testPathPattern integration.test",
    "test:integration": "docker build -t differencify . && docker run --volume \"$(pwd)/src/integration.tests\":/differencify/src/integration.tests -t  differencify npm run jest:integration"
...
NimaSoroush commented 6 years ago

Long term plan would be great to have everything as docker-compose

EranSch commented 6 years ago

Funny, I actually started by creating a docker-compose file because I didn't want to deal with copying files out of the container. I found that I preferred running the container directly because between Jest's console colors being lost and compose's service name eating up horizontal space in the terminal, the test output became difficult to read.

Additionally, I've updated the package.json script to include the jest:integration alias via 1e04926.

NimaSoroush commented 6 years ago

Ok, I see what you mean by that. Let's stick with docker for now then.

Thanks for updating. All LGTM, will merge

NimaSoroush commented 6 years ago

Ahh, We need to update circleCi to not run test:integration and instead run jest:integration

NimaSoroush commented 6 years ago

Otherwise CI failing https://circleci.com/gh/NimaSoroush/differencify/179

EranSch commented 6 years ago

Updated circleCI script, and bumped the version 😀

EranSch commented 6 years ago

Uggghhhh... the paths are different in the detailed output!

image

NimaSoroush commented 6 years ago

So I assume a new PR will all fixes, right?

EranSch commented 6 years ago

All good now via bee9df5a86884872712bdd1d2d3eb4b0a5039dba

NimaSoroush commented 6 years ago

Yeah. All good now. Thanks a lot @Swingline0

I will just publish the latest changes to npm

EranSch commented 6 years ago

Thanks!

NimaSoroush commented 6 years ago

https://www.npmjs.com/package/differencify