E2E Error Output Tests #1502
E2E Error Output Tests #1502
Conversation
| } | ||
|
|
||
| //this ensure we have mutex lock until here | ||
| drop(x); |
There was a problem hiding this comment.
As far as I could tell this drop call is not necessary (the mutex lock will be dropped right after this at the end of the function.
There was a problem hiding this comment.
@itowlson suggested to use explicit drops for better readability.
There was a problem hiding this comment.
Hmm... I understand that when the drop happens early, and you want to show that the lock has been given up before doing more work, but the drop will happen right after this... This strikes me as fairly unidiomatic.
| Err(err) => Err(err), | ||
| } | ||
| let output = utils::run(&["spin", "--version"], None, None)?; | ||
| utils::assert_success(&output); |
There was a problem hiding this comment.
We may eventually remove these assertions in favor of the top-level testing code asserting success. For now though we'll leave this here as every test assumes that these calls succeed.
| @@ -35,7 +35,7 @@ services: | |||
| registry: | |||
| image: ${REGISTRY_IMAGE:-registry:2} | |||
| ports: | |||
| - "5000:5000" | |||
| - "5001:5000" | |||
There was a problem hiding this comment.
I made this change because macOS uses port 5000 for ControlCenter now...
There was a problem hiding this comment.
pkill -f ControlCenter has become my go-to these days.. I think you can stop it altogether, but the change LGTM.
There was a problem hiding this comment.
we need to change this port in tests/testcases/mod.rs as we use it there
There was a problem hiding this comment.
or may be not. because tests run in docker context as well
rylev
force-pushed
the
e2e-errors-messages
branch
5 times, most recently
from
a2db514
to
d6d2c37
Compare
| if let Some(envs) = envs { | ||
| for (k, v) in envs { | ||
| cmd.env(k, v); | ||
| } | ||
| } | ||
|
|
||
| let output = cmd.output()?; | ||
| let code = output.status.code().expect("should have status code"); | ||
| println!("Running command: {cmd:?}"); |
There was a problem hiding this comment.
do we want to move this to debug instead of printing always?
There was a problem hiding this comment.
I originally did this so that it was clear if the tests panicked due to the command failing what the command was (since the command itself is no longer printed in the panic message). I'm not against moving to debug but this might make it less obvious what went wrong when something did. Perhaps we leave it for now, and see how annoying it is before changing it?
|
these changes looks good to me. once again, thank you for putting in time to make improvements here. |
rylev
force-pushed
the
e2e-errors-messages
branch
from
efb8429
to
d6c042c
Compare
|
Annoyingly the broken test consistently works locally. Looks like for some reason there's some extra whitespace produced by |
Signed-off-by: Ryan Levick <ryan.levick@fermyon.com>
Signed-off-by: Ryan Levick <ryan.levick@fermyon.com>
Signed-off-by: Ryan Levick <ryan.levick@fermyon.com>
Signed-off-by: Ryan Levick <ryan.levick@fermyon.com>
Signed-off-by: Ryan Levick <ryan.levick@fermyon.com>
rylev
force-pushed
the
e2e-errors-messages
branch
from
d6c042c
to
52ca63a
Compare
This adds the start of a testing of the error output from spin. The end vision is an easy way for developers to add tests for some sort of broken Spin experience and test the actual stdout/stderr output against some expected output.
We're not quite there, but this PR makes significant changes to the e2e testing framework to allow for test Spin apps to fail with (necessarily) failing the test. I've added one test as well to give an idea of how these tests might work in the future. However, I expect the experience of adding new tests to improve with time as we bake in more convention on how to write such tests.
Note: I did quite a bit of clean up along the way which might make this somewhat difficult to review. I apologize for this, but I hope the intention is still clear. Please let me know if you need more explanation of the changes, and I can write up a more thorough summary.