Skip to content

Forward stdout/stderr from parallel tests #2163

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

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

Conversation

hadley
Copy link
Member

@hadley hadley commented Aug 4, 2025

(100% pure claude code)

I guess the question here is do we want to forward as we update the number of tests, or only at the end when we report the test failures etc. I can see arguments for either

Fixes #2095

I guess the question here is do we want to forward as we update the number of tests, or only at the end when we report the test failures etc. I can see arguments for either

Fixes #2095
@hadley hadley requested a review from gaborcsardi August 4, 2025 00:51
@gaborcsardi
Copy link
Member

I am afraid that it will take a bit more than this. We need to read out the output as we go, (nearly) immediately as it happens, because otherwise the pipe between the processes will be full and the subprocess stops.

This is possibly already happening in the CI jobs that are stuck.

@hadley
Copy link
Member Author

hadley commented Aug 4, 2025

I was afraid that this solution was too simple.

@gaborcsardi
Copy link
Member

gaborcsardi commented Aug 4, 2025

I have a simple solution, the output looks like this:

❯ testthat::test_local(filter = "b")
Starting 2 test processes
✔ | F W  S  OK | Context
⠇ [ FAIL 0 | WARN 0 | SKIP 0 | PASS 0 ] @ bare                                                                                                                  
test-bare.R: hello!
✔ |          5 | bare                                                                                                                                           
✔ |         12 | expect-invisible                                                                                                                               
⠹ [ FAIL 0 | WARN 0 | SKIP 0 | PASS 19 ] @ describe                                                                                                             
test-describe.R:   [1]   1   2   3   4   5   6   7   8   9  10  11  12  13  14  15  16  17  18
test-describe.R:  [19]  19  20  21  22  23  24  25  26  27  28  29  30  31  32  33  34  35  36
test-describe.R:  [37]  37  38  39  40  41  42  43  44  45  46  47  48  49  50  51  52  53  54
test-describe.R:  [55]  55  56  57  58  59  60  61  62  63  64  65  66  67  68  69  70  71  72
test-describe.R:  [73]  73  74  75  76  77  78  79  80  81  82  83  84  85  86  87  88  89  90
test-describe.R:  [91]  91  92  93  94  95  96  97  98  99 100
✔ |          8 | describe                                                                                                                                       
✔ |         15 | quasi-label                                                                                                                                    
✔ |         38 | reporter-debug                                                                                                                                 
                                                                                                                                                                
══ Results ═════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════
Duration: 1.7 s

[ FAIL 0 | WARN 0 | SKIP 0 | PASS 78 ]
Screenshot 2025-08-04 at 5 30 11 PM

I lumped together the standard output and error, and also didn't make an effort to hide the progress bar before printing the output. There is a message() in test-bare.R and an print(1:100) in test-describe.R.

Considering that this is mainly for print-debugging, this might be good enough. It is not any better for non-parallel runs, I think.

@gaborcsardi
Copy link
Member

gaborcsardi commented Aug 4, 2025

  • Add tests
  • Remove the print debugging
  • Fix parallel-setup test case
  • Fix parallel-startup test case

@hadley
Copy link
Member Author

hadley commented Aug 4, 2025

Yeah, that looks perfect to me. Thanks!

Now that we read out stdout/stderr explicitly, we might read it
out before the error is reported from a failed worker startup.
Or if a task is a startup and not a test file (`path == NA`), then
we put the stdout/stderr aside and use it in the error message.
@gaborcsardi
Copy link
Member

@hadley This is done, I think, if you want to take another look. (I can't formally request a review from you, I guess because you are the original author?)

@gaborcsardi gaborcsardi changed the title ✨ Forward stdout/stderr from parallel tests ✨ Forward stdout/stderr from parallel tests Aug 5, 2025
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.

Forward stderr/stdout from parallel tests
2 participants