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

PR #25502 breaks manually run unittests #25613

Closed
Mutantpenguin opened this issue Mar 22, 2024 · 6 comments · Fixed by #26028
Closed

PR #25502 breaks manually run unittests #25613

Mutantpenguin opened this issue Mar 22, 2024 · 6 comments · Fixed by #26028
Assignees

Comments

@Mutantpenguin
Copy link
Contributor

Mutantpenguin commented Mar 22, 2024

Description of the issue

PR #25502 broke our unittests. The corresponding issue is #25496. (I actually don't understand what's going on here)

We run unittests with coverage for our app with the following command and upload the results to GitLab (so this is run through a GitLab CI pipeline). The problem can be recreated locally with the same command:
python -m coverage run --source ../apps/<our_app>/<our_app> -m xmlrunner discover -v -s ../apps/<our_app>/<our_app> -p test_*.py --output-file "$UNITTEST_XML_RESULTS_FILE"

This worked great with frappe 12, 13 and 14 until you merged the PR #25502.

Context information (for bug reports)

Output of bench version

erpnext 14.65.7
frappe 14.68.1

Steps to reproduce the issue

We have inherited our own class from FrappeTestCase and added a frappe.init (and other small necessary stuff) to it to be able to run each unittest individually.
Yes, this is possible through bench run-tests, but we want to use the functionality integrated into VSCode to have visual feedback for each unittest and be able to debug them properly (see https://code.visualstudio.com/docs/python/testing, works like a breeze and we will never want to loose this).
Running the unittests integrated into VSCode poses no problem btw., but this has to do with the fact that the integrated ones only use unittest, and not coverage and xmlrunner.

So for reproduction you need to use the following class for your unitttests and run the aforementioned command:

import random
from typing import final

import frappe
from frappe.database.database import Database
from frappe.tests.utils import FrappeTestCase


class MyTestCase(FrappeTestCase):
	@classmethod
	def setUpClass(cls) -> None:
		# We can't allow any commits, not even in calls from inside of frappe.
		Database.commit = _do_nothing

		cls.TEST_SITE = "site1.tests"

		cls.SHOW_TRANSACTION_COMMIT_WARNINGS = True

		# Check if test site exists
		if cls.TEST_SITE not in frappe.utils.get_sites():
			raise Exception(f"The site '{cls.TEST_SITE}' does not exist. The unit tests can therefore not be executed.")

		# Initialise the site
		frappe.init(site=cls.TEST_SITE)

		# Only continue when tests are allowed for the current site
		if not frappe.local.conf.get("allow_tests"):
			raise Exception(f"The site '{cls.TEST_SITE}' does not allow tests. The unit tests can therefore not be executed.")

		frappe.flags.in_test = True

		if not frappe.db:
			frappe.connect()

		super().setUpClass()

	@final
	def setUp(self) -> None:
		frappe.set_user("Administrator")

		# Inherited classes can implement "my_setup"
		# They must do this because "setUp" is declared final
		if hasattr(self, "my_setup"):
			try:
				self.my_setup()
			except Exception as exc:
				bar_char_list = [ "░", "▒", "▓", "█" ]
				bar_top = "".join(random.choice(bar_char_list) for _ in range(125))
				bar_bottom = "".join(random.choice(bar_char_list) for _ in range(125))

				print(f"\n\n{bar_top}\nThere is an exception in my_setup: {type(exc)} -> {exc}\n{bar_bottom}\n\n")  # noqa: T201
				raise

	@final
	def tearDown(self) -> None:
		# Inherited classes can implement "my_tear_down"
		# They must do this because "tearDown" is declared final
		if hasattr(self, "my_tear_down"):
			self.my_tear_down()

		# Delete all created data
		frappe.db.rollback()

def _do_nothing(*_args, **_kwargs) -> None:  # type: ignore  # noqa: ANN002, ANN003
	pass

Observed result

Unittests run without a problem.

Expected result

The first unittest crashes.

Stacktrace / full error message

Slightly adjusted to omit our app:

Traceback (most recent call last):
  File "/home/frappe/frappe-bench/apps/<our_app>/<our_app>/utils/unittest/my_test_case.py", line 35, in setUp
    frappe.init(site=self._TEST_SITE)
  File "/home/frappe/frappe-bench/apps/frappe/frappe/__init__.py", line 258, in init
    _register_fault_handler()
  File "/home/frappe/frappe-bench/apps/frappe/frappe/__init__.py", line 2405, in _register_fault_handler
    faulthandler.register(signal.SIGUSR1)
io.UnsupportedOperation: fileno

Additional information

We would be happy to throw away our customizations if there is any other sane and integrated way to run and debug each unittest individually through VSCode.

@ankush
Copy link
Member

ankush commented Mar 23, 2024

io.UnsupportedOperation: fileno

This just looks like there's no stderr to print errors to ? Is your setup doing anything funky with stdout/stderr?

@ankush
Copy link
Member

ankush commented Mar 23, 2024

Can you maybe print value of sys.stderr just before that line to see if it's tempered with by some library/tool.

def _register_fault_handler():
	import sys

	print(sys.stderr, type(sys.stderr))
	faulthandler.register(signal.SIGUSR1)

We also use coverage.py but our tests are passing fine - https://github.com/frappe/frappe/actions/runs/8397844336

@ankush
Copy link
Member

ankush commented Mar 23, 2024

(I actually don't understand what's going on here)

We are registering a signal handler. When SIGUSR1 is sent to process we dump stacktrace to sys.stderr (the default)

@ankush ankush removed the bug label Mar 23, 2024
@Mutantpenguin
Copy link
Contributor Author

Mutantpenguin commented Mar 25, 2024

The only real difference I see is us using xmlrunner as a command line tool which seems to tamper with stderr.

This is the output with your print statement:

<xmlrunner.result._DuplicateWriter object at 0x7f220a94d4b0> <class 'xmlrunner.result._DuplicateWriter'>

Edit:
xmlrunner does tamper with stderr xmlrunner/unittest-xml-reporting#194

@Mutantpenguin
Copy link
Contributor Author

For now, we monkey patch frappe._register_fault_handler with an empty function during our unittests.

@ankush ankush self-assigned this Apr 18, 2024
@ankush
Copy link
Member

ankush commented Apr 18, 2024

This is a good idea to add to core - running tests without bench.

@ankush ankush changed the title PR #25502 breaks manually run unittests Allow running tests directly using unittest Apr 18, 2024
ankush added a commit to ankush/frappe that referenced this issue Apr 18, 2024
@ankush ankush changed the title Allow running tests directly using unittest PR #25502 breaks manually run unittests Apr 18, 2024
mergify bot pushed a commit that referenced this issue Apr 18, 2024
mergify bot pushed a commit that referenced this issue Apr 18, 2024
ankush added a commit that referenced this issue Apr 18, 2024
)

* fix: register faulthandler on true stderr only

#25613
(cherry picked from commit a49189a)

* fix: response logging

if response is 4xx then `Response` object itself is getting logged,
which is stupid. This is because of `res and res.text` and res is falsy
if response is not "OK".

(cherry picked from commit 4c85c20)

---------

Co-authored-by: Ankush Menat <ankush@frappe.io>
ankush added a commit that referenced this issue Apr 18, 2024
)

* fix: register faulthandler on true stderr only

#25613
(cherry picked from commit a49189a)

* fix: response logging

if response is 4xx then `Response` object itself is getting logged,
which is stupid. This is because of `res and res.text` and res is falsy
if response is not "OK".

(cherry picked from commit 4c85c20)

---------

Co-authored-by: Ankush Menat <ankush@frappe.io>
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 4, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants