Skip to content

Conversation

@lee-ji-an
Copy link

@lee-ji-an lee-ji-an commented Aug 5, 2025

What is this PR for?

This PR fixes the IRInterpreterTest.testZShow() test failure caused by R 4.0+ default behavior change.
The test was failing because R 4.0+ changed the default stringsAsFactors from TRUE to FALSE, causing the test to expect numeric factor levels but receive actual string values.

What type of PR is it?

Bug Fix

Todos

  • - Update test expectation to match R 4.0+ default behavior
  • - Ensure test passes on both R 3.x and R 4.x versions

What is the Jira issue?

How should this be tested?

  • Run ./mvnw test -pl rlang -Dtest=IRInterpreterTest#testZShow
  • Verify test passes on both R 3.x and R 4.x environments

Screenshots (if appropriate)

N/A

Questions:

  • Does the license files need to update? No
  • Is there breaking changes for older versions? No
  • Does this needs documentation? No

@ParkGyeongTae
Copy link
Member

Could you please update the PR title to be a complete sentence? This helps provide clearer context when browsing the commit or PR history. Thanks!

@lee-ji-an lee-ji-an changed the title [ZEPPELIN-6212] Fix IRInterpreterTest.testZShow() for R 4.0+ compatib… [ZEPPELIN-6212] Fix IRInterpreterTest.testZShow() for R 4.0+ compatibility Aug 7, 2025
@lee-ji-an
Copy link
Author

Could you please update the PR title to be a complete sentence? This helps provide clearer context when browsing the commit or PR history. Thanks!

Thank you for the review! I've updated the PR title to be a complete sentence as requested.

@jongyoul
Copy link
Member

jongyoul commented Aug 8, 2025

@lee-ji-an I think we'd better add a test case for both environments using github actions.

@lee-ji-an
Copy link
Author

lee-ji-an commented Aug 10, 2025

@lee-ji-an I think we'd better add a test case for both environments using github actions.

@jongyoul I agree that testing both R 3.x and R 4.x environments would provide better coverage.
I'll add a GitHub Actions matrix to test both R versions and update the PR. Thank you for the thoughtful feedback!

- r-ggplot2
- r-irkernel
- r-shiny
- r-googlevis No newline at end of file
Copy link
Member

Choose a reason for hiding this comment

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

Please add a newline at the end of this file.

Copy link
Member

@ParkGyeongTae ParkGyeongTae left a comment

Choose a reason for hiding this comment

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

LGTM 👍

assertEquals("<font color=red>Results are limited by 1 rows.</font>\n",
resultMessages.get(1).getData());
}
} No newline at end of file
Copy link
Member

Choose a reason for hiding this comment

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

Please add a blank line at the end of this file.

- r-ggplot2
- r-irkernel
- r-shiny
- r-googlevis
Copy link
Member

Choose a reason for hiding this comment

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

Please add a blank line at the end of this file.

- r-ggplot2
- r-irkernel
- r-shiny
- r-googlevis
Copy link
Member

Choose a reason for hiding this comment

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

Please add a blank line at the end of this file.

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.

3 participants