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

Fix issue with basilisp test standard streams output #1082

Merged

Conversation

ikappaki
Copy link
Contributor

@ikappaki ikappaki commented Oct 9, 2024

Hi,

can you please review patch to address the basilisp test failure on MS-Windows. It fixes #1080.

The problem arises because pytest's mechanism for capturing standard streams during tests breaks when those streams are saved before pytest.main is called and then used in the test. This happens when running basilisp test from the CLI:

In basilisp.init, sys.stdout and sys.stderr are saved to the *out* and *err* dynamic variables before pytest.main is called. If a test prints anything, it tries to write to these original streams, causing failures due to an issue I just reported in pytest-dev/pytest#12876.

This patch rebinds *out* and *err* to the streams set by pytest and restores them when pytest finishes (via the configure functions). I've tested it with a case that prints to *out* and *err* at the top level, inside a test, and inside a fixture, and it works as expected.

(ns tests.issuetests.issue-test
  (:require [basilisp.test :refer [deftest are is testing use-fixtures]])
  (:import sys))

(println :top-out)
(binding [*out* *err*]
  (println :top-err))

(defn fixture-gen-test []
  (println :fix-gen-out)
  (yield)
  (binding [*out* *err*]
    (println :fix-gen-err)))

(use-fixtures :each fixture-gen-test)

(deftest print_test
  (println :test-out)
  (is (= 5 5))
  (binding [*out* *err*]
    (println :test-err)))
> basilisp test
=============================================================================================================== test session starts ================================================================================================================
platform win32 -- Python 3.11.4, pytest-8.4.0.dev100+g410c51011, pluggy-1.5.0
rootdir: C:\clj\issues\issue-bas-test-macro-print
configfile: pyproject.toml
plugins: basilisp-0.2.4
collecting ... :top-out
:top-err
collected 1 item

tests\issuetests\issue_test.lpy :fix-gen-out
:test-out
:test-err
.:fix-gen-err

I’d also like to discuss Basilisp integration tests, which I think the above testing falls under, though there’s currently no support for such tests in Basilisp.

Ideally I think, I would like to create a Basilisp project in a temporary directory, add the above test, install the fix alongside pytest, activate the environment, and then run basilisp test.

Do you have any suggestions how to extend the Basilisp test suite to implement this? or shall I open a new ticket for this discussion? I was thinking of having a new --integration pytest option for these tests, something I have been experimenting with in the basilisp-blender package.

Thanks

@ikappaki ikappaki force-pushed the issue/bas-pytest-capture-mswin branch from 80a068d to 0b88a04 Compare October 10, 2024 20:15
@ikappaki ikappaki marked this pull request as ready for review October 10, 2024 20:31
Copy link
Member

@chrisrink10 chrisrink10 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 you'll need to rewrite the changes below in terms of push-thread-bindings and pop-thread-bindings because otherwise the changes you're pushing into *out* and *err* here won't be eligible for binding conveyance.

@ikappaki
Copy link
Contributor Author

I think you'll need to rewrite the changes below in terms of push-thread-bindings and pop-thread-bindings because otherwise the changes you're pushing into *out* and *err* here won't be eligible for binding conveyance.

Updated. I've used list comprehension for brevity when creating the dictionary, but I can change it if you think it's not worth it.

@ikappaki
Copy link
Contributor Author

The report coverage failure doesn't seem to be related. It was unable to install tox, most likely an intermittent issue?

@chrisrink10
Copy link
Member

chrisrink10 commented Oct 14, 2024

The report coverage failure doesn't seem to be related. It was unable to install tox, most likely an intermittent issue?

It appears to be related to this: actions/runner-images#10781 (indeed unrelated to your changes).

@chrisrink10 chrisrink10 merged commit f91d59f into basilisp-lang:main Oct 14, 2024
11 of 12 checks passed
@ikappaki
Copy link
Contributor Author

The report coverage failure doesn't seem to be related. It was unable to install tox, most likely an intermittent issue?

It appears to be related to this: actions/runner-images#10781

Good catch—talk about being at the forefront of cutting edge technology! 😅 A viable option could be to switch from ubuntu-latest to ubuntu-22.04 until a solution is proposed in that ticket, while maintaining the current status quo..

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.

basilisp test fails when attempting to print anything out from inside a test file on MS-Windows
2 participants