Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

handle error based on exit code #2 #77

Merged
merged 3 commits into from
Jul 13, 2019

Conversation

Steveb-p
Copy link
Contributor

@Steveb-p Steveb-p commented May 29, 2019

Fixes tests related to #75 change and includes the original commit. Rebased due to conflict with release in package-lock.json.

Changes:

  • close event now properly rejects on exit code other than 0 (BREAKING CHANGE)
  • Tests are now migrated to Jest
  • Some tests did not clean properly, which was the reason why tests started failing
  • Allow omission of options argument (falls back to empty object)

@Steveb-p Steveb-p force-pushed the 74-error-handling branch from 2e7663f to dcc2b6a Compare May 29, 2019 21:38
@Steveb-p
Copy link
Contributor Author

Steveb-p commented May 29, 2019

@AlexZeitler I've tried to find what is wrong with that --workdir test, but it seems that the error only occurs on CI? I'm kinda exhausted at this point and have run out of ideas :P

It seems it's unable to switch working directory properly.

You can see it in output in the last run for this PR. I'm leaving output set to true in that test until it's resolved.

On a side note, it might be worth it to stop rejecting when any message comes from stderr. It's probably not how we want it to behave and always wait for exit event instead.

Also, I'll try to force git to respect index.js -> index.test.js as rename, but I'll do that after CI test issue resolution as well.

@Steveb-p Steveb-p force-pushed the 74-error-handling branch from f2a6738 to dfd44ce Compare July 13, 2019 11:18
@Steveb-p
Copy link
Contributor Author

@AlexZeitler apparently CircleCI docker executor is incapable of mounting volumes to docker containers.

https://support.circleci.com/hc/en-us/articles/360007324514-How-can-I-mount-volumes-to-docker-containers-

I'm wondering how the tests worked previously with those mounts?

Since the related test is basically failing due to inability to mount a directory, I'll remove the need for mounting and check a system file instead. Is that ok?

@AlexZeitler
Copy link
Contributor

@Steveb-p ok

@Steveb-p
Copy link
Contributor Author

I'm pushing an updated tests then and see if it works.

Please DO NOT merge it, I'll rebase and squash all those commits into one.

Always `down` containers after test
Fix workdir test failing on CircleCI
See: https://support.circleci.com/hc/en-us/articles/360007324514-How-can-I-mount-volumes-to-docker-containers-
@Steveb-p Steveb-p force-pushed the 74-error-handling branch from cd10088 to 5cc5ced Compare July 13, 2019 11:50
@Steveb-p
Copy link
Contributor Author

@AlexZeitler done. It should now properly pass all tests and should be ready for merging.

@AlexZeitler AlexZeitler merged commit 46c50b3 into PDMLab:master Jul 13, 2019
@AlexZeitler
Copy link
Contributor

@Steveb-p Great job, thanks! 👌
📦 is on npm @ 0.19.0

@Steveb-p Steveb-p deleted the 74-error-handling branch April 9, 2021 22:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants