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

Improve error message when forced fail test fails #354

Open
wants to merge 35 commits into
base: main
Choose a base branch
from

Conversation

WillGibson
Copy link
Contributor

@WillGibson WillGibson commented Dec 30, 2024

I started with something small relating to #349 in order to start familiarising myself with the Mutmut code/tests.

@@ -885,14 +885,14 @@ def print_stats(source_file_mutation_data_by_path, force_output=False):
print_status(f'{(s.total - s.not_checked)}/{s.total} 🎉 {s.killed} 🫥 {s.no_tests} ⏰ {s.timeout} 🤔 {s.suspicious} 🙁 {s.survived} 🔇 {s.skipped}', force_output=force_output)


def run_forced_fail(runner):
def run_forced_fail_test(runner):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed to better differentiate it from the runner's run_force_fail method.

print("FAILED")
os._exit(1)
print("Error: Unable to force a test failure during the forced fail test")
raise SystemExit(1)
Copy link
Owner

Choose a reason for hiding this comment

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

I would suggest "FAILED: Unable to force test failures". To be consistent, and for brevity.

Why did you change the exit? I use os._exit() for fast exit, so we don't spend time deallocating at the end of the process.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have update the error message.

I switched to SystemExit because it is much easier to write a test for. Re the performance thing, I think this is something that will only happen once, early on in a run, if something has gone wrong, so some milliseconds taken occasionally doesn't seem like a problem.

Copy link
Contributor Author

@WillGibson WillGibson Dec 30, 2024

Choose a reason for hiding this comment

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

Not sure why test_run_forced_fail_test_with_all_tests_passing is passing locally, but not in GitHub Actions. I'll come back to that when I next have some time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All the unit tests I added are failing in GitHub Actions because the output emitted by run_forced_fail_test is not making it to stdout.

I have tried adding some sleeps to rule out a race condition and changing the runner to MacOS in case that was a factor. No banana.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@boxed I have poked around and set things up so that there is a test that does not capture the output and running single tests with verbose etc.

If you look at this GitHub Action run, it seems that CatchOutput is only capturing the first line of the output in GitHub Actions for some reason. You can see "done" (and "FAILED" for come reason without the new text) in the test output.

My time for playing this morning has run out. If you have any thoughts on why this might be I would be interested in hearing them.

Copy link
Contributor Author

@WillGibson WillGibson Jan 15, 2025

Choose a reason for hiding this comment

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

Temporarily running the tests on MacOS as well as Ubuntu in GitHub Actions indicates it's not an OS difference.

I'm now wondering whether there is some difference in the way https://github.com/actions/setup-python installs Python compared to how I installed it (via asdf) on my laptop.

@boxed (or anyone else) If you checkout this branch and run the following, does it pass or fail, and how did you install Python?

python -m pytest -vvv --capture=no tests/test_mutation.py::test_run_forced_fail_test_with_failing_test

Copy link
Owner

@boxed boxed Jan 17, 2025

Choose a reason for hiding this comment

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

It passes. I use basic venv and pip.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. But... damn. I wonder what the difference is in GitHub Actions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I comment out the with CatchOutput part in run_forced_fail_test, capfd works as expected.

I suspect CatchOutput and capfd are doing much the same thing behind the scenes and CatchOutput is usurping capfd. I'll need to read the source code for capfd, but boarding a plane imminently.

@boxed What is the intent/purpose of CatchOutput?

Copy link
Owner

Choose a reason for hiding this comment

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

Yea that makes sense.

The purpose is to not show a lot of output when running mutmut, but being able to dump it on errors. Showing all output to the user would make the tool basically unusable.

@WillGibson WillGibson force-pushed the improve-failed-forced-fail-error branch from 2edd3ea to 70cbf80 Compare December 31, 2024 11:15
@WillGibson WillGibson force-pushed the improve-failed-forced-fail-error branch from b2a014d to 6fd2e3b Compare January 14, 2025 02:55
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