Add integration tests for 'stable env' #73
Conversation
Having not found a way to make `@chdir[I32]` or anything like that work,
this commit adds a bash helper script that `cd`s into the directory
before running stable.
Also includes the code changes necessary to create the temporary dir
from pony, and remove it after the tests are done.
The former is done so that upcoming tests can use pony to set up some
initial situation (such as drop a bundle.json).
The cleanup is not done in a `tear_down()` method, because I found no
non-clumsy way to get the reference to the directory into that spot. So
instead, this has a mini-actor that deletes the temp dir when disposed,
and is set up to be called by the test helper.
Note that there's no strict need to run the existing integration tests
in their own temp dirs, so we might make that temp dir passing optional,
maybe, by changing `Exec`'s `cleaner arg from
cleaner: DisposableActor
to
cleaner: (DisposableActor | None)
and pass None for help and version.
Signed-off-by: Stephan Renatus <[email protected]>
Turns out this is probably the lowest-hanging fruit right now. This makes use of the previous commit's "run in temp dir" feature. Signed-off-by: Stephan Renatus <[email protected]>
…ent) This is in anticipation of what's going to change in ponylang#68. Signed-off-by: Stephan Renatus <[email protected]>
srenatus
force-pushed
the
sr/integration-test/env
branch
from
cbab081
to
a27efe9
Compare
| h.long_test(100_000_000) | ||
| let tmp = | ||
| try | ||
| FilePath.mkdtemp(h.env.root as AmbientAuth, "")? |
There was a problem hiding this comment.
❔ The "" is a bit icky, as it'll create the temp dir in the current working directory -- i.e., the checkout of pony-stable. It'll get cleaned up, but it still strikes me as a questionable default. (OTOH, I'm not sure if /tmp is much better, but I might be overthinking this. We could also create an empty tmp dir in the repo, and use that...)
|
I am fine with creating a temporary directory in the current folder in general as long as it is guaranteed to not clash with anything existing. I wont have much time in the next months but ill try to have a look at this soonish. Thanks for your efforts in bringing stable forward. |
This adds stable/test/integration/tmp/ as an empty, tracked folder; and uses that one as base directory for running the integration tests. It felt a bit cleaner to me than having the temp dirs created in the top-level folder; also causes less of a mess when debugging integration tests by disabling the cleanup dispose()'s delete call. (I suppose it would have been just as good to use /tmp/, but I wasn't sure what our expectations regarding different operating systems were.) Signed-off-by: Stephan Renatus <[email protected]>
|
🤔 I'm a little concerned that this has failed on CircleCI for some reason -- this indicates that the tests are flakey, which will be annoying long-term. However, I can run |
|
Huh, this happened again right away. Trying to dig in... Running the tests locally using the Running this in a loop for a while; let's see if the failure comes up locally... Update: ran this 25 times with no issue. (It takes a lot longer than a Next up: limiting the docker resources locally to the CircleCI specs -- 4G mem, 2 cpu -- and running Update: no issues after having run that ^ 100 times. Trying to restrict resources further, I've found that giving the job the minimum memory possible in docker, 4M, reproducibly gives timeouts: (Note that this only works if you've run However, bumping the timeouts to 1s even makes these go away when running in 4M mem containers. I would thus think that this could be a compromise. The hypothesis is that a timeout of 1s is enough; and the next commit, bumping timeouts further, shall make this PR pass on circleci. |
|
@srenatus i would suggest running the tests using the --sequential flag. That will run them one at a time and should fix your timeout problem. Then they aren't competing for resources, you can also put them in an exclusion group together so no more than 1 member of an exclusion group runs at a time. --sequential (like we do for ponyc tests) is probably your best option. |
|
@SeanTAllen good idea! Thanks 🎉 Update: note those two commits below -- seems like a modest timeout bump (200ms) and running sequentially makes this OK. In any case, it's something to keep an eye on. If this becomes too annoying (it shouldn't impede contributions), I'm happy to revisit it. |
One of these tests had timed out on circleci. Having flakey tests seems an annoyance we should try to avoid; but I couldn't reproduce this by running `make integration` in a loop locally. So, the hypothesis is that the timeout was no bug in the tests, but a genuine timeout. Trying to reproduce the test failures locally, I've found that restricting the memory to 4M (docker's minimum) caused the tests to fail in a reproducible way. The integration test binary is now passed `--sequential`, so that the individual test runs don't compete for resources. This makes the error _not appear anymore_ in the docker 4M scenario. This reverts commit 8d8a906.
srenatus
force-pushed
the
sr/integration-test/env
branch
from
562cc0a
to
80713eb
Compare
Seems like --sequential alone isn't enough. 200ms doesn't seem too bad, so let's try this. Signed-off-by: Stephan Renatus <[email protected]>
stable/test/integration/helper.sh
Outdated
| @@ -0,0 +1,3 @@ | |||
| #!/bin/bash | |||
| cd "$CWD" || exit 100 | |||
There was a problem hiding this comment.
this looks like a hack. While this is fine, this shows that ProessMonitor should support setting the cwd like e.g. the python subprocess.Popen allows. I will create an issue in the ponyc repo for this.
There was a problem hiding this comment.
I created ponylang/ponyc#2821 maybe link this somewhere in your code, so we track it and be able to remove this hack once ProcessMonitor supports setting the cwd.
stable/test/integration/_exec.pony
Outdated
| let pm: ProcessMonitor = ProcessMonitor(auth, auth, consume notifier, | ||
| path, consume args, consume vars) | ||
| pm.done_writing() | ||
| h.dispose_when_done(pm) | ||
| h.dispose_when_done(cleaner) |
There was a problem hiding this comment.
i think the test creating the tmpdir should be responsible for cleaning it up. So just call this right after the temporary directory has been created.
Signed-off-by: Stephan Renatus <[email protected]>
|
Hiccups on travis, I've tried re-running. If it doesn't do the trick, I can deal with this later. (Tomorrow morning the latest.) |
Signed-off-by: Stephan Renatus <[email protected]>
|
Ok the travis issue has resolved itself -- added a note re: the ponyc issue, will merge this when green. |
Turns out this is probably the lowest-hanging fruit right now. This makes
use of the previous commit's "run in temp dir" feature.
Part of #28.
Happy to discuss the approach taken in this PR's first commit -- using the shell wrapper to
cdinto the right place. I've tried getting this to work in some other way -- using an exclusion group for side-stepping concurrency issues in@chdir, but couldn't get it to work. After that, the present solution seemed OK enough 😉Curious about all the input I can get here
:)What I dislike most is the duplication in the new env tests, but I couldn't come up with a better structure.