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

[ci] [R-package] ensure that MSVC jobs fail when tests fail (fixes #5439) #5448

Merged
merged 38 commits into from
Sep 12, 2022

Conversation

jameslamb
Copy link
Collaborator

Fixes #5439.

@jameslamb
Copy link
Collaborator Author

In b9eebb3, I tried intentionally causing a unit test failure, and this change:

# before
Run-R-Code-Redirect-Stderr "source('testthat.R')" ; Check-Output $?

# after
Rscript.exe --vanilla "testthat.R" ; Check-Output $?

Saw that both CI jobs failed and reported test failures in logs (build link).

Then, in 7ce18e2, tried reverting the intentional test failure. Interestingly, the windows-2022 job passed and the windows-2019 job failed. Both should have passed.

  • windows-2019, MSVC, R 3.6, cmake (build link)
  • windows-2022, MSVC, R 4.1, cmake (build link)

I'll investigate this more tomorrow.

@jameslamb
Copy link
Collaborator Author

iiiiinterresting!

commenting out all of the dataset.R tests results in the windows-2019 job succeeding!

@jameslamb jameslamb changed the title WIP: [ci] [R-package] ensure that MSVC jobs fail when tests fail (fixes #5439) [ci] [R-package] ensure that MSVC jobs fail when tests fail (fixes #5439) Sep 8, 2022
@jameslamb
Copy link
Collaborator Author

jameslamb commented Sep 8, 2022

Alright, this is ready for review!

I believe the root cause of the original issue (#5439) is something in the intersection between:

  • how the library compiled with MSVC interacts with stdout + stderr + R's output streams
  • how {testthat} interacts with R's output streams
  • the way that powershell sometimes treats output to stderr as a failure (see discussion from [ci] Move R Windows CI to GitHub Actions #3168)

This PR proposes two changes to ensure that MSVC R-package CI jobs actually fail when the tests fail:

  • using Rscript.exe testthat.R directly to run the tests, instead of the Run-R-Code-Redirect-Stderr powershell function
  • skipping one test in test_dataset.R that only fails on windows-2019, MSVC, R 3.6, cmake job

Why I think it's ok to skip that one test only on that one job

  • that test is passing on all other operating systems and versions of R tested in CI
    • including, especially, the combination of R 3.6 + Windows + cmake build (just with MinGW instead)
  • the time spent investigating this one test on an old Visual Studio version + old powershell version + old R version could be better spent working on other items for the 4.0.0 release ([RFC] 4.0.0 Release #5153)
  • re-enabling these two CI jobs will give LightGBM coverage of R + cmake + MSVC in CI again (which it currently doesn't really have, since all jobs silently pass)

Evidence that the jobs will fail as of these changes

See the results from this commit which intentionally made one R unit test fail: aff9acf.

  • windows-2019, MSVC, R 3.6, cmake (build link)
  • windows-2022, MSVC, R 4.1, cmake (build link)

@jameslamb jameslamb marked this pull request as ready for review September 8, 2022 02:29
Copy link
Collaborator

@StrikerRUS StrikerRUS left a comment

Choose a reason for hiding this comment

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

Thanks for working on this!

LGTM except two minor suggestions below:

.ci/test_r_package_windows.ps1 Show resolved Hide resolved
R-package/tests/testthat/test_dataset.R Outdated Show resolved Hide resolved
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity since it was closed. To start a new related discussion, open a new issue at https://github.com/microsoft/LightGBM/issues including a reference to this.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[R-package] [ci] unit test failures don't cause MSVC CI jobs to fail
2 participants