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 tests #1273

Merged
merged 6 commits into from
Jan 7, 2025
Merged

Fix tests #1273

merged 6 commits into from
Jan 7, 2025

Conversation

ccleva
Copy link
Contributor

@ccleva ccleva commented Jan 3, 2025

Thanks for contributing.

Description

  • Correct DataFrameJoinerTest.leftOuterEmptyLeftTable to use JoinType.LEFT_OUTER instead of the default INNER - inner join is tested in innerJoinEmptyLeftTable
  • Added assertions in AggregateFunctionsTest.testMultipleColumnTypes and DoubleArraysTest.testTo2dArray

Testing

Did you add a unit test?

@benmccann
Copy link
Collaborator

Can you rebase this PR to make sure the formatter runs against it now that the formatter's been fixed?

@ccleva
Copy link
Contributor Author

ccleva commented Jan 3, 2025

I rebased it without issue in local, but the new formatter gives me an error related to jvm version with both eclipse and command line. I need to see how to fix this before any other contribution.

@benmccann
Copy link
Collaborator

Hmm. That's too bad. I have Java 11 installed for java and javac. What version do you have? I'd hate if it's causing issues for everyone who wants to contribute

@ccleva
Copy link
Contributor Author

ccleva commented Jan 4, 2025

Hmm. That's too bad. I have Java 11 installed for java and javac. What version do you have? I'd hate if it's causing issues for everyone who wants to contribute

I'm using java 17 for command line, and latest eclipse requires (and provides) java 21. The issue happens for java 16+, see https://github.com/Cosium/git-code-format-maven-plugin?tab=readme-ov-file#jdk-16-peculiarities.

The recommended configuration solves the issue. This should be documented somewhere for contributors using java 16+.

@ccleva
Copy link
Contributor Author

ccleva commented Jan 4, 2025

Rebased, commit hooks ran, and force-pushed. Should be fine now.

@benmccann
Copy link
Collaborator

This should be documented somewhere for contributors using java 16+.

Agreed. Is this something we can update in the pom.xml or it should just go in the CONTRIBUTING.md? Either way, mind sending a PR for that? You're better able to test it since you're on a newer Java than I am

@benmccann benmccann merged commit 48b5f69 into jtablesaw:master Jan 7, 2025
7 checks passed
@ccleva
Copy link
Contributor Author

ccleva commented Jan 7, 2025

Agreed. Is this something we can update in the pom.xml or it should just go in the CONTRIBUTING.md? Either way, mind sending a PR for that? You're better able to test it since you're on a newer Java than I am

You have to add jvm options to maven, typically in the .mvn/jvm.config file in the repo. Would you prefer a commented-out option file in the repo and the documentation stating it needs to be uncommented for java 16+, or only the description of what needs to be done ? In any case CONTRIBUTING.md seems the right place.

@benmccann
Copy link
Collaborator

I wonder if we can just add that file and leave it uncommented. Hopefully it won't have any ill effects for Java 11 users. I could test that if you share the file

@ccleva ccleva deleted the fix_tests branch January 7, 2025 17:31
@ccleva
Copy link
Contributor Author

ccleva commented Jan 7, 2025

I'll make a PR like you suggest, if it does not work with java 11 we will explore other options

@ccleva
Copy link
Contributor Author

ccleva commented Jan 7, 2025

Done in #1280. It did not make the builds with java 11 fail, but I would be more confident if you test it in local.

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.

2 participants