ponylang / pony-stable

:horse: A simple dependency manager for the Pony language.
BSD 2-Clause "Simplified" License
134 stars 18 forks source link

Start integration test scaffolding #70

Closed srenatus closed 6 years ago

srenatus commented 6 years ago

This is for #28. It's not nearly complete, but I'd appreciate some feedback on the approach 😃

Am I missing something obvious? (Are there any better "integration-y test" examples to follow?)

I though about making use of other frameworks (like, kyua, rspec, ...) but then, having it all done in pony has a certain appeal.

Also, I've split this off of #68 -- I'll add tests for the behaviour changed there when this is done.

srenatus commented 6 years ago

@jemc if you have a minute, I'd appreciate a your input :) -- my current tiny plan is to get this merged, then add to the tests for #68.

srenatus commented 6 years ago

@jemc thank you. I've updated the branch. (Note that the way I had approached matching std{out,err} was buggy -- the fix is commit 9b0427caf14a1698451d02057471d976177af996.)

srenatus commented 6 years ago

I've just tried something along those lines here https://github.com/srenatus/pony-stable/commit/3f56a9eb8ae2baa8918b8e7555138da3f2d8d524

Ran into the binary location issue there, too. Env car for that might do the trick, too :)

On Fri, 15 Jun 2018, 14:36 Matthias Wahl, notifications@github.com wrote:

@mfelsche commented on this pull request.

In stable/test/integration/_exec.pony https://github.com/ponylang/pony-stable/pull/70#discussion_r195718315:

+use "process" +use "regex" + +actor _Exec

  • new create(
  • h: TestHelper,
  • cmdline: String,
  • notifier: ProcessNotify iso)
  • =>
  • try
  • let args = cmdline.split_by(" ")
  • let path = FilePath(h.env.root as AmbientAuth, "build/release/stable")?
  • let vars: Array[String] iso = []
  • let auth = h.env.root as AmbientAuth
  • let pm: ProcessMonitor = ProcessMonitor(auth, auth, consume notifier,
  • path, consume args, consume vars)

Just an idea:

I think there is no way to do this currently (correct me if i'm wrong) but wouldn't it be nice to execute the binary with PWD in a temporary directory, possibly prepopulated with some bundle.json or something else or nothing at all. And afterwards assert on the directory contents in one way or the other.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/ponylang/pony-stable/pull/70#pullrequestreview-129144366, or mute the thread https://github.com/notifications/unsubscribe-auth/AA1I7ri1H49DZz8QFsRSmjEQJoDubzguks5t86pAgaJpZM4UoML8 .

srenatus commented 6 years ago

@mfelsche (besides rebasing), I've added a commit taking care of the binary path thing using an env var (https://github.com/ponylang/pony-stable/pull/70/commits/ccd473635382287811f6039d6264380e39cbaeb2).

Next up (my plan for next week's dabbling):

I'm not certain that this is going to work (that's half the fun), but it feels preferable over what I've sketched above (here, having the "run in directory" logic put into the Makefile).

At some point, it might be nice to get this merged, and go on from there -- while I still enjoy adding to these tests, clinging to this one PR might exclude others from contributing. 🤔 But I'm also OK with keeping this around until it's more complete.

jemc commented 6 years ago

I agree that it is better to merge sooner instead of continually increasing the scope of the PR, with the ideal goal being that each PR just implements one unit of change.

srenatus commented 6 years ago

@jemc I've fixed the whitespace, and updated https://github.com/ponylang/pony-stable/issues/28#issuecomment-397951512 to keep track of remaining integration test needs -- to be resolved outside of this PR.

Let's get this merged -- I'll just wait for an approval, and will then combine all those commit messages to something more succinct when squash'n'merging.

srenatus commented 6 years ago

@mfelsche thanks for the 👀

Went ahead and got this in. Hope this is OK -- I've assumed that there's no need to update the changelog for this, as a user doesn't perceive any changes from that -- would y'all agree?

mfelsche commented 6 years ago

Agreed.