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

eliot "task" for failed tests say "succeeded" #650

Open
meejah opened this issue Feb 25, 2022 · 2 comments
Open

eliot "task" for failed tests say "succeeded" #650

meejah opened this issue Feb 25, 2022 · 2 comments

Comments

@meejah
Copy link
Collaborator

meejah commented Feb 25, 2022

It seems the "status" for the Eliot "task" that is created per test shows "succeeded" as the status for passed as well as failed tests in the integration tests.

@meejah meejah added the bug Something isn't working label Feb 25, 2022
@exarkun
Copy link
Member

exarkun commented Mar 26, 2022

I think this was intentional. The Eliot action succeeds to reflect the fact that the test runner succeeded in "running" the test. The fact that the test itself may have failed is reflected elsewhere.

The per-test Eliot action comes from this code:

https://github.com/LeastAuthority/magic-folder/blob/9b5292c639fe6896c7c95f1e680c5a64b5b09734/src/magic_folder/test/eliotutil.py#L150-L156

The eliot_logged_test decorator starts and stops the action. As long as the decorated run method does not raise an exception, the action is a success. The run method is not supposed to raise exceptions - that isn't part of the TestCase interface. If the test has failures or errors, they're recorded on the result object.

The Eliot action for the test could be made to fail by inspecting the result object after the application code for the test runs (the inner run function being called does that) to find out if it had any errors or failures. It would probably be tricky to make this change with the code factored exactly as it is now because the decorated run method should not raise an exception but that is the only way it has to signal to the action object in eliot_logged_test that something has failed.

Rather than trying to do this refactoring, I'd dig deeper into why it's useful to have these actions marked as failures. Maybe there's a good reason and the refactoring is worthwhile, or maybe there's an easier path to accomplish whatever the goal is.

@meejah
Copy link
Collaborator Author

meejah commented Mar 26, 2022

I believe the motivation was simply trying to understand what is going on (and it was surprising to me that a "failed" test says "action: succeeded").

For example, looking for the logs relating to a single failed test in hundreds of KB of eliot logs. It's really hard to find any useful information since the eliot-tree parser doesn't produce things chronologically, lumping lots of stuff together (but also doesn't put everything related to one test in one "action"/tree) while eliot-prettyprint becomes tedious (and formats exceptions poorly too).

@meejah meejah added needs-investigation and removed bug Something isn't working labels Mar 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants