GH-38570: [R] Ensure that test-nix-libs is warning free #38571

jonkeane · 2023-11-03T14:30:50Z

Rationale for this change

Although we stop / fail the CI job for warnings elsewhere, we don't seem to be doing that for our buildsystem tests. Let's change that so warnings + other output doesn't creep in.

Prevent *** Failed to find latest nightly for 8.0.0.9000 from showing up when the file is sourced under test
fix Warning ('test-nixlibs.R:140:3'): select_binary() with test program 'x' is NULL so the result will be NULL
update version strings to be characters to avoid r-devel warnings

What changes are included in this PR?

Added stop_on_warning to the test call, and fixes to make that pass

Are these changes tested?

Yes, they are all part of the test suite

Are there any user-facing changes?

No.

Closes: [R] Ensure that test-nix-libs is warning free #38570

r/tools/nixlibs.R

jonkeane · 2023-11-03T17:55:29Z

r/tools/nixlibs.R

+if (is_release & !test_mode) {
  VERSION <- VERSION[1, 1:3]
  arrow_repo <- paste0(getOption("arrow.repo", sprintf("https://apache.jfrog.io/artifactory/arrow/r/%s", VERSION)), "/libarrow/")
-} else {
+} else if(!test_mode) {


We could also wrap this whole block in if (!test_mode) { ... } since we don't want to run the actual detection yet, just check the functions. Unless I'm missing something and we actually do want this whole main chunk to run?

I think we do want the whole main chunk to run (or at least, from spending a few minutes with the script last week it seemed like that was the original intention). Wrapping the main logic in a function and testing that function would be another way to go about it but I don't feel to strongly about it (I'm just glad there's a place to put tests for these functions).

And just to make sure I understand: you're suggesting leaving this as is rather than making the whole block skip while being tested.

We could also suppress the message more deeply (or even in the test script) but IMHO both of those are harder to figure out what's up / reason about than stopping this one bit from running during testing and producing distracting output. But happy to change it to elsewhere if you have strong feelings about it

I think most of the distracting output is fixed in #38534 even with sourcing the script (output here: https://github.com/apache/arrow/actions/runs/6746935491/job/18341961924?pr=38534#step:5:23756 ). Is there any part of that output that could be improved?

This bit here is about suppressing this part, which I see in that output:

*** No nightly binaries were found for version 8.0.0.9000: falling back to libarrow build from source

We could also set LIBARROW_BINARY=false?

I also just used the existing pattern so 🤷 I think it should be fine to fix it like this as further up is a block that sets the version specifically for testing and iirc we don't use the arrow repo option in any of the tests (other wise we would also see fails in ci).

jonkeane · 2023-11-03T20:32:22Z

cc @paleolimbot in case this overlaps with anything you're working on

paleolimbot · 2023-11-05T01:16:34Z

I'll take a closer look Monday! I did have #38534 open to solves one of the two warnings listed but I'm also happy to rebase after this one merges 🙂

paleolimbot

Thank you for taking this on! I think I'd prefer to keep the existing "run the whole script when in test mode" logic even though it's a little odd (unless @assignUser corrects me and it was never the original intention to run the entire script in the test block).

paleolimbot

I remembered what I was thinking of: the comment # Prevent error with binary selection during testing. vs. the comment # Flag so that we just load the functions and don't evaluate them like we do when called from configure.R. I'll add a comment in #38534 to clarify that we don't actually want to run selection in the test file! (Or feel free to add here!)

assignUser

LGTM thanks

conbench-apache-arrow · 2023-11-07T23:54:36Z

After merging your PR, Conbench analyzed the 5 benchmarking runs that have been run so far on merge-commit 7622ded.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 5 possible false positives for unstable benchmarks that are known to sometimes produce them.

…#38571) ### Rationale for this change Although we stop / fail the CI job for warnings elsewhere, we don't seem to be doing that for our buildsystem tests. Let's change that so warnings + other output doesn't creep in. - [x] Prevent `*** Failed to find latest nightly for 8.0.0.9000` from showing up when the file is sourced under test - [x] fix `Warning ('test-nixlibs.R:140:3'): select_binary() with test program 'x' is NULL so the result will be NULL` - [x] update version strings to be characters to avoid r-devel warnings ### What changes are included in this PR? Added `stop_on_warning` to the test call, and fixes to make that pass ### Are these changes tested? Yes, they are all part of the test suite ### Are there any user-facing changes? No. * Closes: apache#38570 Authored-by: Jonathan Keane <jkeane@gmail.com> Signed-off-by: Jacob Wujciak-Jens <jacob@wujciak.de>

### Rationale for this change Although we stop / fail the CI job for warnings elsewhere, we don't seem to be doing that for our buildsystem tests. Let's change that so warnings + other output doesn't creep in. - [x] Prevent `*** Failed to find latest nightly for 8.0.0.9000` from showing up when the file is sourced under test - [x] fix `Warning ('test-nixlibs.R:140:3'): select_binary() with test program 'x' is NULL so the result will be NULL` - [x] update version strings to be characters to avoid r-devel warnings ### What changes are included in this PR? Added `stop_on_warning` to the test call, and fixes to make that pass ### Are these changes tested? Yes, they are all part of the test suite ### Are there any user-facing changes? No. * Closes: #38570 Authored-by: Jonathan Keane <jkeane@gmail.com> Signed-off-by: Jacob Wujciak-Jens <jacob@wujciak.de>

…#38571) ### Rationale for this change Although we stop / fail the CI job for warnings elsewhere, we don't seem to be doing that for our buildsystem tests. Let's change that so warnings + other output doesn't creep in. - [x] Prevent `*** Failed to find latest nightly for 8.0.0.9000` from showing up when the file is sourced under test - [x] fix `Warning ('test-nixlibs.R:140:3'): select_binary() with test program 'x' is NULL so the result will be NULL` - [x] update version strings to be characters to avoid r-devel warnings ### What changes are included in this PR? Added `stop_on_warning` to the test call, and fixes to make that pass ### Are these changes tested? Yes, they are all part of the test suite ### Are there any user-facing changes? No. * Closes: apache#38570 Authored-by: Jonathan Keane <jkeane@gmail.com> Signed-off-by: Jacob Wujciak-Jens <jacob@wujciak.de>

Stop on warnings

e566e78

jonkeane requested review from assignUser, kou and raulcd as code owners November 3, 2023 14:30

github-actions bot added the awaiting committer review Awaiting committer review label Nov 3, 2023

jonkeane added 2 commits November 3, 2023 09:58

Prevent NULL in ifelse from warning

b4bd90c

Pacakge versions should be characters

2141d13

github-actions bot added the Component: R label Nov 3, 2023

jonkeane commented Nov 3, 2023

View reviewed changes

r/tools/nixlibs.R Show resolved Hide resolved

jonkeane commented Nov 3, 2023

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Nov 3, 2023

paleolimbot reviewed Nov 6, 2023

View reviewed changes

paleolimbot approved these changes Nov 6, 2023

View reviewed changes

github-actions bot added awaiting merge Awaiting merge and removed awaiting changes Awaiting changes labels Nov 6, 2023

assignUser approved these changes Nov 7, 2023

View reviewed changes

assignUser merged commit 7622ded into apache:main Nov 7, 2023
28 checks passed

assignUser removed the awaiting merge Awaiting merge label Nov 7, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-38570: [R] Ensure that test-nix-libs is warning free #38571

GH-38570: [R] Ensure that test-nix-libs is warning free #38571

jonkeane commented Nov 3, 2023 •

edited

Loading

jonkeane Nov 3, 2023

paleolimbot Nov 6, 2023

jonkeane Nov 6, 2023

paleolimbot Nov 6, 2023

jonkeane Nov 6, 2023

paleolimbot Nov 6, 2023

assignUser Nov 7, 2023

jonkeane commented Nov 3, 2023

paleolimbot commented Nov 5, 2023

paleolimbot left a comment

paleolimbot left a comment

assignUser left a comment

conbench-apache-arrow bot commented Nov 7, 2023

GH-38570: [R] Ensure that test-nix-libs is warning free #38571

GH-38570: [R] Ensure that test-nix-libs is warning free #38571

Conversation

jonkeane commented Nov 3, 2023 • edited Loading

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

jonkeane Nov 3, 2023

Choose a reason for hiding this comment

paleolimbot Nov 6, 2023

Choose a reason for hiding this comment

jonkeane Nov 6, 2023

Choose a reason for hiding this comment

paleolimbot Nov 6, 2023

Choose a reason for hiding this comment

jonkeane Nov 6, 2023

Choose a reason for hiding this comment

paleolimbot Nov 6, 2023

Choose a reason for hiding this comment

assignUser Nov 7, 2023

Choose a reason for hiding this comment

jonkeane commented Nov 3, 2023

paleolimbot commented Nov 5, 2023

paleolimbot left a comment

Choose a reason for hiding this comment

paleolimbot left a comment

Choose a reason for hiding this comment

assignUser left a comment

Choose a reason for hiding this comment

conbench-apache-arrow bot commented Nov 7, 2023

jonkeane commented Nov 3, 2023 •

edited

Loading