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

Numeric Array Formula and Matrix Function Patch Applied #69

Closed
wants to merge 7 commits into
base: trunk
from

Conversation

Projects
None yet
3 participants
@Bob95132

Bob95132 commented Aug 30, 2017

Follow up to the request for pull request format of patch

@pjfanning

This comment has been minimized.

Show comment
Hide comment
@pjfanning

pjfanning Aug 30, 2017

I get 3 test failures in TestWorkbookEvaluator with NullPointerExceptions due to ec.getWorkbook() being null.

I get 3 test failures in TestWorkbookEvaluator with NullPointerExceptions due to ec.getWorkbook() being null.

@pjfanning

This comment has been minimized.

Show comment
Hide comment
@pjfanning

pjfanning Aug 30, 2017

I have added the test xlsx file and added the ant and gradle build changes necessary for commons-math3 at Bob95132#1 - would you be able to merge these to your branch?

pjfanning commented Aug 30, 2017

I have added the test xlsx file and added the ant and gradle build changes necessary for commons-math3 at Bob95132#1 - would you be able to merge these to your branch?

Merge pull request #1 from pjfanning/bug-61469
Add missing xlsx file and add ant and gradle dependencies on commons-…
@Bob95132

This comment has been minimized.

Show comment
Hide comment
@Bob95132

Bob95132 Aug 30, 2017

I apologize for the late response and thank you for those corrections. Should I supply a new patch including these changes to bugzilla or is the pull request sufficient? Is another pull request to Apache/POI needed?

Bob95132 commented Aug 30, 2017

I apologize for the late response and thank you for those corrections. Should I supply a new patch including these changes to bugzilla or is the pull request sufficient? Is another pull request to Apache/POI needed?

@pjfanning

This comment has been minimized.

Show comment
Hide comment
@pjfanning

pjfanning Aug 30, 2017

We can use this Pull Request and your existing github branch while we review this change.
We are in the middle of releasing 3.17 and we need to decide whether to include this.
Would you be able to fix the broken tests?

pjfanning commented Aug 30, 2017

We can use this Pull Request and your existing github branch while we review this change.
We are in the middle of releasing 3.17 and we need to decide whether to include this.
Would you be able to fix the broken tests?

@Bob95132

This comment has been minimized.

Show comment
Hide comment
@Bob95132

Bob95132 Aug 30, 2017

For clarification, correcting the failed tests in TestWorkbookEvaluator? Did this error occur during a full system JUnit run and were there other failures?

Bob95132 commented Aug 30, 2017

For clarification, correcting the failed tests in TestWorkbookEvaluator? Did this error occur during a full system JUnit run and were there other failures?

@pjfanning

This comment has been minimized.

Show comment
Hide comment
@pjfanning

pjfanning Aug 30, 2017

the tests in TestWorkbookEvaluator fail in the IDE or if you run them using ant clean test - the NullPointerException happens in code that you are adding

pjfanning commented Aug 30, 2017

the tests in TestWorkbookEvaluator fail in the IDE or if you run them using ant clean test - the NullPointerException happens in code that you are adding

Fixed TestWorkbookEvaluator by creating mock evaluation workbook that…
… corresponds to test parameters set (sheet, row, and column indices)
@Bob95132

This comment has been minimized.

Show comment
Hide comment
@Bob95132

Bob95132 Aug 30, 2017

I have fixed the broken tests. Please let me know if there is anything else I can do.

Bob95132 commented Aug 30, 2017

I have fixed the broken tests. Please let me know if there is anything else I can do.

@pjfanning

This comment has been minimized.

Show comment
Hide comment
@pjfanning

pjfanning Aug 30, 2017

I see another test failure:

junit.framework.AssertionFailedError: wrong # of valid values expected:<32> but was:<0>
at org.apache.poi.xssf.usermodel.TestXSSFDataValidation.testTableBasedValidationList(TestXSSFDataValidation.java:354)

pjfanning commented Aug 30, 2017

I see another test failure:

junit.framework.AssertionFailedError: wrong # of valid values expected:<32> but was:<0>
at org.apache.poi.xssf.usermodel.TestXSSFDataValidation.testTableBasedValidationList(TestXSSFDataValidation.java:354)

@pjfanning

This comment has been minimized.

Show comment
Hide comment
@pjfanning

pjfanning Aug 31, 2017

Would it also be possible to add some tests for an xls file (HSSF format)?

pjfanning commented Aug 31, 2017

Would it also be possible to add some tests for an xls file (HSSF format)?

@Bob95132

This comment has been minimized.

Show comment
Hide comment
@Bob95132

Bob95132 Aug 31, 2017

Im working on the TestXSSFDataValidation and will make an xls file test suite. I have noticed that test suites between xls and xlsx are sometimes duplicated. Is it sufficient to keep the test cases within the workbook the same?

Bob95132 commented Aug 31, 2017

Im working on the TestXSSFDataValidation and will make an xls file test suite. I have noticed that test suites between xls and xlsx are sometimes duplicated. Is it sufficient to keep the test cases within the workbook the same?

@pjfanning

This comment has been minimized.

Show comment
Hide comment
@pjfanning

pjfanning Aug 31, 2017

running similar tests is fine - it's just that we need to have an xls input file as well as an xlsx - the test classes themselves should be separate - one for HSSF and one for XSSF

pjfanning commented Aug 31, 2017

running similar tests is fine - it's just that we need to have an xls input file as well as an xlsx - the test classes themselves should be separate - one for HSSF and one for XSSF

@Bob95132

This comment has been minimized.

Show comment
Hide comment
@Bob95132

Bob95132 Aug 31, 2017

Thanks for the clarification

Bob95132 commented Aug 31, 2017

Thanks for the clarification

Bobby Hulbert and others added some commits Sep 12, 2017

@Bob95132

This comment has been minimized.

Show comment
Hide comment
@Bob95132

Bob95132 Sep 12, 2017

I have added the tests for the HSSFWorkbook and corrected the failure that was occurring with TestXSSFDataValidation. Please let me know the next steps in evaluating this contribution.

Bob95132 commented Sep 12, 2017

I have added the tests for the HSSFWorkbook and corrected the failure that was occurring with TestXSSFDataValidation. Please let me know the next steps in evaluating this contribution.

@pjfanning

This comment has been minimized.

Show comment
Hide comment
@pjfanning

pjfanning Sep 12, 2017

We're in the process of releasing 3.17. I will look at merging this once it is released.

pjfanning commented Sep 12, 2017

We're in the process of releasing 3.17. I will look at merging this once it is released.

@asfgit asfgit closed this in 5ef2f16 Sep 13, 2017

@mzdravkov

This comment has been minimized.

Show comment
Hide comment
@mzdravkov

mzdravkov Jan 11, 2018

Hello, what is the status of this MR? Is there any chance it'll be merged soon?

mzdravkov commented Jan 11, 2018

Hello, what is the status of this MR? Is there any chance it'll be merged soon?

@pjfanning

This comment has been minimized.

Show comment
Hide comment
@pjfanning

pjfanning Jan 11, 2018

merged on Sep 14, 2017

pjfanning commented Jan 11, 2018

merged on Sep 14, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment