exoframejs / exoframe

Exoframe is a self-hosted tool that allows simple one-command deployments using Docker
https://exoframejs.github.io/exoframe/
1.13k stars 56 forks source link

Default ignores/.exoframeignore doesn't work on Windows #289

Closed FDiskas closed 4 years ago

FDiskas commented 4 years ago

Lets try this one

Closes #284

yamalight commented 4 years ago

Seems like you fixed the paths issue, but broke quite a few tests. Why did you remove those console.log.restore()'s?

FDiskas commented 4 years ago

Console restore IMO should be after test. In case of exeption or test failure console is not restored. Also in case --watch it also errors about it

FDiskas commented 4 years ago

Also tests are failed in local computer on mac. Installed nothing changed ant fails. :/

yamalight commented 4 years ago

@FDiskas currently it requires CI=true env var to make tests pass because of the changes to chalk that strips colors only in CI (still haven't gotten around to fixing that). On restoring it after all tests - that's possible but would require full test suite rewrite. So is making it usable via --watch. If you want to give it a shot - be my guest :)

FDiskas commented 4 years ago

Also in bedt scenario console should be restored afterEatch And also use jest spyon

it('calls console.log with "hello"', () => { const   consoleSpy = jest.spyOn(console, 'log'); console.log('hello'); expect(consoleSpy).toHaveBeenCalledWith('hello'); });
FDiskas commented 4 years ago

About CI=true This could be set as ENV in repo settings

yamalight commented 4 years ago

@FDiskas there is 102 different little things that could be improved within this project - using jest.spyOn instead of sinon, making tests --watch-friendly, migrating from inquirer to more modern equirer, etc, etc, etc. And I'm all up for that, but don't have enough time for just about anything. If you want to tackle those - as I've said, be my guest! But please make a separate PR for each of those if you have time for that :)

Re: CI flag - it shouldn't be needed at all, it was a dirty temporary fix to ship a hotfix for CLI. We already have testing environment setting that should strip colors by default. Again - I just didn't have time to address that.

FDiskas commented 4 years ago

@Tiim Reverted tests - sorry

yamalight commented 4 years ago

@FDiskas looks good, thank you! ❤️

github-actions[bot] commented 4 years ago

Coverage Status

Coverage remained the same at 81.091% when pulling a3446cc13708a20ee453ea1381e9890eeee4d43a on FDiskas:bugfix/ignore-path-on-windows into 568969c73132e644391e21ba8defaf429589b4d8 on exoframejs:master.