Skip to content
This repository has been archived by the owner on Aug 12, 2020. It is now read-only.

process.cwd() conflicts in cleanup #4

Closed
CodeLenny opened this issue Jul 23, 2017 · 11 comments
Closed

process.cwd() conflicts in cleanup #4

CodeLenny opened this issue Jul 23, 2017 · 11 comments

Comments

@CodeLenny
Copy link
Collaborator

CodeLenny commented Jul 23, 2017

cleanup() nukes all database storage directories in process.cwd(), which would likely cause conflicts between files.

In newer versions of AVA, process.cwd() returns the directory containing package.json, not the directory containing the test file (avajs/ava#1074), which makes the problem even worse, as all test files will conflict with each other.

I'm planning to make a series of changes to update ava-rethinkdb, including fixing this issue.

  • ava-21

    • Update ava in devDependency to use 0.21.0, the latest version
    • Add ava to peerDependencies with versions 0.21.x and < 0.21.0, so users can't use newer versions of AVA with breaking changes.
    • Add a .travis.yml file for Travis CI testing, testing against Node.js 6 and 8 and AVA v0.15.2 and v0.21.0
  • test-instances

    • Add AVA tests that use ava-rethinkdb from multiple instances, to simulate the final example in the README
  • fix-cwd

    • Update init() and cleanup() to be less dependent on process.cwd()
      Potentially use ${process.cwd()}/${process.pid}/${port}, and then nuke ${process.pid} in cleanup()

Let me know if these changes are appealing, and I'll work on some PRs! I'm currently just drafting changes in my fork, so I'll probably need to take a second pass to make sure that I conform to your style.

@rrdelaney
Copy link
Owner

They all sounds good to me 🙂 I don't really work on or use this project anymore, so any updates are very welcome ✨

@CodeLenny
Copy link
Collaborator Author

Glad to hear it! Expect some PRs then 😄

@CodeLenny
Copy link
Collaborator Author

@rrdelaney PRs #5-#7 have been created. Once everything is merged and ava-rethinkdb is at a stable point, it might make sense to commit the generated package-lock.json file for stability.

@rrdelaney
Copy link
Owner

Is there a reason to commit package-lock.json for libraries?

@CodeLenny
Copy link
Collaborator Author

@rrdelaney I'm not sure, I haven't used lockfiles much. It would ensure that testing is in a consistent environment, and fully inform end-users which version we installed for passing tests, if they have concerns over library mis-matches.

@rrdelaney
Copy link
Owner

I think it would be better to test without a package-lock.json, that we we use the latest matching semver we allow.

@rrdelaney
Copy link
Owner

Thanks for cleaning all of this up 😅 I've added you as a collaborator on GitHub for the future 🙂 I don't use this anymore, and the more maintenance the merrier! Do you have an account on npm? I can give you publishing rights to the package

@CodeLenny
Copy link
Collaborator Author

@rrdelaney Sorry, I've been on vacation and lost track of this :)

You're welcome! Happy to be a collaborator. I'm @CodeLenny on npm.

@rrdelaney
Copy link
Owner

Hope the vacation was good! I'll give you publishing rights on npm tonight. I haven't released any of your changes, but feel free to do so

@CodeLenny
Copy link
Collaborator Author

@rrdelaney vacation was great, thanks! I'll publish the changes in the near future.

@CodeLenny
Copy link
Collaborator Author

@rrdelaney I've published the changes, so I'll close this issue.

It looks like there might be some other bugs that I'm hitting, but I'll open other issues/PRs to deal with those if needed.

Thanks!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants