Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support a non-exiting run mode #39

Merged
merged 1 commit into from
Oct 8, 2024

Conversation

itowlson
Copy link
Contributor

@itowlson itowlson commented Oct 1, 2024

The Spin runtime tests use conformance-tests as a library (https://github.com/fermyon/spin/blob/967fdf368612478cef176bccc491faffad680050/tests/runtime.rs#L31). However, run_tests entry point calls libmimic::Conclusion::exit(), which causes the test process to exit at the end of the tests, with the result of the libmimic tests only. This means that if a Spin runtime test fails but all conformance tests succeed, the overall test suite still passes, disguising the test failure unless the avid reader pores over the logs.

This PR proposes splitting the run_tests entry point into run_tests_and_exit, which retains the current behaviour, and run_tests_to_conclusion, which instead returns the Conclusion object. libmimic still prints all progress and results as normal.

With this in place, the Spin runtime test that checks conformance will be able to examine Conclusion::has_failed and propagate failure (e.g. by returning an error such as "One or more conformance tests failed") or pass on success, without interfering with other tests.

Of course the names and behaviour are totally up for grabs (some names are deliberate to make consumers aware of changed behaviour, but that may be misguided), and if you feel there's a better way of managing this then please feel free to circular-file this - I'm not at all familiar with the code base or with other use cases I'm afraid.

Copy link
Collaborator

@rylev rylev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is overall a good idea. I think it would personally be better to keep the naming the same and just move everything to return a libtest_mimic::Conclusion. The caller then just needs to call .exit() if that's their desired behavior. I think that accomplishes everything we want here without making the public API surface more complicated or verbose. What do you think?

@itowlson
Copy link
Contributor Author

itowlson commented Oct 7, 2024

@rylev My concern with that was that a consumer who was calling run_tests and relying on the exit behaviour in the Ok case would get no hint that they now needed to call exit() themselves. Changing the name would give them a compilation error alerting them that something had changed. I agree having a function that called exit for them was probably excessive though!

But I'm guessing from your comment that we (you) know and control all consumers and this won't be an issue! So I've set the name back as you suggest with just the change to the return type.

@itowlson itowlson requested a review from rylev October 7, 2024 18:36
@itowlson
Copy link
Contributor Author

itowlson commented Oct 8, 2024

@rylev I know you have other things going on but do you have a moment to look at this please? It should be a quick yes/no and it would unblock the corresponding Spin PR. Thanks!

@rylev
Copy link
Collaborator

rylev commented Oct 8, 2024

Sorry this took longer to review than it should have!

@rylev rylev merged commit ecd22a4 into fermyon:main Oct 8, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants