Stop looking for bundle.json if an invalid one is found

[srenatus]

The previous behaviour was a little irritating:

If invoked in /some/nested/dir, stable would try to read

/some/nested/dir/bundle.json

and if that failed (because the file didn't exist or was invalid), it would try to read, subsequently,

/some/nested/bundle.json
/some/bundle.json
/bundle.json

This implied that errors reading any found bundle.json were printed, but stable kept trying to find a valid one, and eventually, having reached /bundle.json and assuming it didn't exist, would print

No bundle.json in current working directory or ancestors.

With this change, the logic is different: stable will first try to find an existing bundle.json, and then attempt to read it. If it fails, it will not go on, but stop.

---

Questions

Q: How to test this?

• Some test that actually invokes the built stable binary given different input situations could include a case of this "nested with broken bundle.json at the bottom" test.
• The function _locate_bundle() alone could be tested if it was split off the Main actor. Is there a pattern to follow for this?
  • I can't have a constructor for Bundle what would be able to differentiate "keep going" from "stop it" errors. (Where an existing file with a bad format would trigger a stop; and no-file-found would make it "keep going".)
  • Is there such as thing as a static method? Or would we create a "Utils" class for this?

A: I went with the last option, introducing a primitive BundleLocator, and adding tests for that. I've failed to hide it better, as calling it _BundleLocator made it impossible to import it in tests, it seems?

[mfelsche]

What you could do to test a package private class/actor/whateveryouwantbasically is to create a _test.pony file in the source directory and create a TestList in there, that is not private, that is not a Main actor and that has a make constructor. In this file you can add tests for private apis and add them to the TestList. In the test package you can then instantiate that TestList from _test.pony and run its test.

As an example: https://github.com/ponylang/http/blob/master/http/_test.pony and https://github.com/ponylang/http/blob/master/http/test/main.pony

This way you can have both tests for private and public apis.

[srenatus]

@mfelsche thank you! I'll update this PR in a bit. (I had actually looked at that http code yesterday, but it didn't "click". Your words did it, however 👍)

I generally wonder how we approach "hiding" in pony-stable -- I mean, I suppose we could hide much more of its current API surface; but also, I wonder if we need to. I think this isn't meant to be used as a library, so we probably aren't too strict about it. (This could be an argument against adapting BundleLocator to be a private primitive, but I still prefer that, and want to adapt the PR.)

[srenatus]

Don't merge ⚠️ this isn't quite right for the case where there's no bundle.json.

Before: it would create one. Now: it fails. The None case of the match here needs adaptation. I'll take care of that.

[srenatus]

✔️ This could be good to go. Since this is currently still lacking any integration-y tests, I suppose we could either scrutinize this PR thoroughly, or hold it off until we have some.

On that topic, I'd be happy to help. It seems like #28 got a little stuck?

⏩ I could imagine either adding tests for Main() with a pre-created Env (pointing to some temp dir, if that's possible), then checking the outcome. Alternatively, building the binary and testing it by some other means (either another ponytest suite, or something like bats) would seem useful as well. What do you think?

[SeanTAllen]

@srenatus I think that some integration tests as part of this would be a great addition to the PR. If you want to pick up #28, that would be awesome.

[srenatus]

@SeanTAllen I've split off getting integration test scaffolding in: #70 ☝️

[SeanTAllen]

@srenatus sounds good to me.

[mfelsche]

@srenatus this needs to be rebased.

[srenatus]

@mfelsche thanks for the heads-up; by current plan is leave this rot^Wwaiting until #73 is in -- since that would at least give a little assurance that this isn't breaking the world. :)

[srenatus]

@mfelsche rebased and updated ✔️

[mfelsche]

booyah!