NIFI-11778 refactored Groovy tests in nifi-elasticsearch-restapi-processors to Java (and JUnit 5) #7537

dan-s1 · 2023-07-28T17:15:35Z

Summary

NIFI-11778 / NIFI-9290
Refactored all the Groovy unit tests in nifi-elasticsearch-restapi-processors to Java.
Along with the refactor, the building of the individual JSON documents were removed and replaced with files which are now read in thereby reducing the size of the unit tests and increasing the readability of the unit tests.

Tracking

Please complete the following tracking steps prior to pull request creation.

Issue Tracking

Apache NiFi Jira issue created

Pull Request Tracking

Pull Request title starts with Apache NiFi Jira issue number, such as NIFI-00000
Pull Request commit message starts with Apache NiFi Jira issue number, as such NIFI-00000

Pull Request Formatting

Pull Request based on current revision of the main branch
Pull Request refers to a feature branch with one commit containing changes

Verification

Please indicate the verification steps performed prior to pull request creation.

Build

Build completed using mvn clean install -P contrib-check
- JDK 17

Licensing

New dependencies are compatible with the Apache License 2.0 according to the License Policy
New dependencies are documented in applicable LICENSE and NOTICE files

Documentation

Documentation formatting appears as expected in rendered files

exceptionfactory · 2023-07-28T17:30:31Z

Thanks for putting in the work on this large refactoring effort @dan-s1!

@ChrisSamo632 and @MikeThomsen have put in a good bit of work on the Elasticsearch components, so they would be in a good position to evaluate these changes.

dan-s1 · 2023-07-28T18:03:11Z

@exceptionfactory I see Ubuntu Zulu JDK 17 EN and i-workflow / Windows Zulu JDK 17 FR failed but the errors are not related to the code I checked in. The failure on ci-workflow / MacOS Temurin JDK 17 JP is related to my code but I am not sure why it is failing as on my box it is working fine. I am guessing there is a timezone issue though I would like to see the whether the other builds also fail or not to determine conclusively.

ChrisSamo632 · 2023-07-29T12:13:52Z

@dan-s1 I re-triggered the Checks, definitely looks like ES related assertion failures now (originally you were right that the tests for for some unrelated transient issue)

Error:  Failures: 
Error:    PutElasticsearchRecordTest.testDateTimeFormatting:600 org.opentest4j.AssertionFailedError: expected: <2> but was: <0>
Error:    PutElasticsearchRecordTest.testRecordPathFeatures:337 org.opentest4j.AssertionFailedError: expected: <1> but was: <0>
Error:  Tests run: 124, Failures: 2, Errors: 0, Skipped: 0
Error:  Failed to execute goal org.apache.maven.plugins:maven-surefire-plugin:3.0.0:test (default-test) on project nifi-elasticsearch-restapi-processors: There are test failures.
Error:  
Error:  Please refer to /home/runner/work/nifi/nifi/nifi-nar-bundles/nifi-elasticsearch-bundle/nifi-elasticsearch-restapi-processors/target/surefire-reports for the individual test results.

ChrisSamo632

Thanks for the big refactor @dan-s1 - couple of code style/suggestions from me, plus worth considering breaking up some of the larger test cases that have grown to be somewhat unmaintainable over time - now seems a sensible opportunity to refactor those

Plus you've some tests failing in the Checks/when run locally that need fixing

Also, this is a duplicate of NIFI-9290

...ch-client-service-api/src/main/java/org/apache/nifi/elasticsearch/IndexOperationRequest.java

...va/org/apache/nifi/processors/elasticsearch/AbstractPaginatedJsonQueryElasticsearchTest.java

...ssors/src/test/java/org/apache/nifi/processors/elasticsearch/PutElasticsearchRecordTest.java

...ocessors/src/test/java/org/apache/nifi/processors/elasticsearch/SearchElasticsearchTest.java

dan-s1 · 2023-08-02T16:01:20Z

@dan-s1 I re-triggered the Checks, definitely looks like ES related assertion failures now (originally you were right that the tests for for some unrelated transient issue)

Error:  Failures: 
Error:    PutElasticsearchRecordTest.testDateTimeFormatting:600 org.opentest4j.AssertionFailedError: expected: <2> but was: <0>
Error:    PutElasticsearchRecordTest.testRecordPathFeatures:337 org.opentest4j.AssertionFailedError: expected: <1> but was: <0>
Error:  Tests run: 124, Failures: 2, Errors: 0, Skipped: 0
Error:  Failed to execute goal org.apache.maven.plugins:maven-surefire-plugin:3.0.0:test (default-test) on project nifi-elasticsearch-restapi-processors: There are test failures.
Error:  
Error:  Please refer to /home/runner/work/nifi/nifi/nifi-nar-bundles/nifi-elasticsearch-bundle/nifi-elasticsearch-restapi-processors/target/surefire-reports for the individual test results.

@ChrisSamo632 Do you have any suggestions how to reproduce these failures? On my local box all unit tests are passing. I thought it might be a locale issue and I tried setting the locale to "FR" with
Locale.setDefault(Locale.FRENCH);
and separately with
Locale.setDefault(Locale.FRANCE);

but the unit test still passed.

dan-s1 · 2023-08-02T16:25:22Z

NM I got it to fail by using this

TimeZone.setDefault(TimeZone.getTimeZone("Europe/Paris"));

dan-s1 · 2023-08-02T16:40:46Z

@ChrisSamo632 Sorry by accident closed this PR and the reopened. Would it be okay to fix the test errors by placing in setUpBeforeClass

TimeZone.setDefault(TimeZone.getTimeZone("UTC")); ?

I realized the timezone was the issue from the second answer in this post.
Both of these tests use

DateTimeFormatter.ofPattern(RecordFieldType.TIMESTAMP.getDefaultFormat()) and I noticed in the javadoc for
RecordFieldType.TIMESTAMP

A timestamp field type. Fields of this type use a {@code java.sql.Timestamp} value.

Based on the aformentioned post I deduced since my default timezone is UTC the tests passed on my box because then LocalDateTime is then the same as Unix time which is UTC. I assume the other boxes where this failed have timezones other than UTC hence the tests fails since at that point java.sql.Timestamp and LocalDateTime are not equivalent.

ChrisSamo632 · 2023-08-02T18:54:59Z

@ChrisSamo632 Sorry by accident closed this PR and the reopened. Would it be okay to fix the test errors by placing in setUpBeforeClass

TimeZone.setDefault(TimeZone.getTimeZone("UTC")); ?

I'm not sure this sounds like the right way to fix this - the Checks run against multiple timezones for good reason, we want to be sure that processors can handle changes in locale. Having to set the tests to use a specific timezone to pass, seems like we might be hiding a potential issue either in the tests or the processor code itself.

Unfortunately, handling multiple timezones isn't my strong suite, although I'll try to take a look. I wonder whether @exceptionfactory or @markap14 might be able to provide some guidance on this one?

exceptionfactory · 2023-08-02T18:59:37Z

@ChrisSamo632 Sorry by accident closed this PR and the reopened. Would it be okay to fix the test errors by placing in setUpBeforeClass
TimeZone.setDefault(TimeZone.getTimeZone("UTC")); ?

I'm not sure this sounds like the right way to fix this - the Checks run against multiple timezones for good reason, we want to be sure that processors can handle changes in locale. Having to set the tests to use a specific timezone to pass, seems like we might be hiding a potential issue either in the tests or the processor code itself.

Unfortunately, handling multiple timezones isn't my strong suite, although I'll try to take a look. I wonder whether @exceptionfactory or @markap14 might be able to provide some guidance on this one?

I will try to take a closer look at the tests soon, but I agree, changing the default timezone in test setup code is not the right approach.

All of the automated builds run with different timezones to ensure expected behavior in various locations, so all tests must pass without changing the system timezone.

In general, the issue means that there is some timezone conversion happening, which could point to a runtime issue, or simply an issue with the sample data used for testing. I'm not sure exactly why this has not surfaced earlier, so it points to an issue with the test-related changes.

...ssors/src/test/java/org/apache/nifi/processors/elasticsearch/PutElasticsearchRecordTest.java

ChrisSamo632

Have suggested some new test case names as requested @dan-s1

...ssors/src/test/java/org/apache/nifi/processors/elasticsearch/PutElasticsearchRecordTest.java

...cessors/src/test/java/org/apache/nifi/processors/elasticsearch/PutElasticsearchJsonTest.java

...ssors/src/test/java/org/apache/nifi/processors/elasticsearch/PutElasticsearchRecordTest.java

...ocessors/src/test/java/org/apache/nifi/processors/elasticsearch/SearchElasticsearchTest.java

ChrisSamo632

Thanks for continuing with the refactor @dan-s1 - a few comments on the lastest commit, but definitely moving in the right direction

dan-s1 · 2023-08-22T15:40:17Z

@ChrisSamo632 Checking in to see if my latest changes are okay. Thanks!

ChrisSamo632 · 2023-08-23T20:07:07Z

@ChrisSamo632 Checking in to see if my latest changes are okay. Thanks!

@dan-s1 I've been away, but hoping to take a look at your latest changes soon/this week.

Note that you've now got some conflicts after #7441 was recently merged - couple of new tests to migrate over for the updated processors

dan-s1 · 2023-08-23T20:17:13Z

@ChrisSamo632 Do I manually cut and paste those changes to my branch since those Groovy files have been deleted in my branch?

ChrisSamo632 · 2023-08-23T20:28:19Z

@ChrisSamo632 Do I manually cut and paste those changes to my branch since those Groovy files have been deleted in my branch?

I'd guess so, and make them work for the Java setup rather than Groovy

dan-s1 · 2023-08-23T20:39:34Z

@exceptionfactory Just double checking before I proceed that the way to resolve the conflicts is for me to cut and paste the changes from #7441 to my code. Thanks!

exceptionfactory · 2023-08-23T21:09:22Z

@exceptionfactory Just double checking before I proceed that the way to resolve the conflicts is for me to cut and paste the changes from #7441 to my code. Thanks!

Yes, copying over and converting the new unit tests is the way to go.

ChrisSamo632

@dan-s1 had a go at more completely suggesting the changes I was thinking about previously to rationalise the testPagination and related methods between the AbstractPaginatedJsonQueryElasticsearchTest and child classes

This, hopefully, shows that these two classes should behave much the same and make it easier to maintain these tests in the future

...va/org/apache/nifi/processors/elasticsearch/AbstractPaginatedJsonQueryElasticsearchTest.java

...ocessors/src/test/java/org/apache/nifi/processors/elasticsearch/SearchElasticsearchTest.java

.../test/java/org/apache/nifi/processors/elasticsearch/PaginatedJsonQueryElasticsearchTest.java

dan-s1 · 2023-08-25T17:50:41Z

@ChrisSamo632 I am trying to implement the changes from #7441 For testBatchingAndErrorRelationship in PutElasticsearchJsonTest.java for the equivalent line of runner.assertTransferCount(PutElasticsearchJson.REL_SUCCESS, 4) is failing as the transfer count is 6 and not 4. Please advise as I do not see any changes in PutElasticsearchJson and the only visible change was to make changes to src/test/resources/PutElasticsearchJsonTest/batchWithError.json.

dan-s1 · 2023-08-25T18:13:42Z

@ChrisSamo632 NM I forgot to account for the changes to src/test/resources/common/sampleErrorResponse.json

dan-s1 · 2023-08-28T14:31:44Z

@ChrisSamo632 I have made the changes you had requested. I also incorporated the changes from #7441. I broke up PutElasticsearchRecordTest#testErrorRelationship into five separate tests

testFailedRecordsOutput
testFailedRecordsOutputGroupedByErrorType
testNotFoundResponsesTreatedAsFailedRecords
testNotFoundFailedRecordsGroupedAsErrorType
testErrorsLoggedWithoutErrorRelationship

and I refactored out the common code into the method testErrorRelationship

ChrisSamo632 · 2023-08-28T18:36:02Z

@dan-s1 I'll take a look shortly hopefully, but note that you've still got some conflicts with main, you'll need to rebase against (or merge in) main

dan-s1 · 2023-08-28T18:46:56Z

@ChrisSamo632 I am not sure I can do anything regarding these conflicts. I already incorporated the changes of #7441 for PutElasticsearchRecord and other related changes. As for the groovy files, naturally there will be a conflict as my branch deleted them and replaced them with Java files.

exceptionfactory · 2023-08-28T18:55:05Z

@ChrisSamo632 I am not sure I can do anything regarding these conflicts. I already incorporated the changes of #7441 for PutElasticsearchRecord and other related changes. As for the groovy files, naturally there will be a conflict as my branch deleted them and replaced them with Java files.

@dan-s1 You will need to rebase your PR and then force-push to this feature branch to resolve the conflicts. If you have already incorporated the changes, then during the rebase process, you should be able to just mark the groovy files as deleted, but the change to PutElasticsearchRecord needs to be incorporated.

…csearch-restapi-processors to Java (and JUnit 5)

…archTest and PaginatedJsonQueryElasticsearchTest and incorporated the changes from apache#7441.

dan-s1 · 2023-08-28T19:03:14Z

@exceptionfactory Thanks for the clarification. I have made those changes and pushed.

ChrisSamo632

LGTM, thanks for continuing with the updates @dan-s1; provided the checks pass (these tests all work for me locally at least), I'm happy to merge

…essors to Java (and JUnit 5) This closes #7537 Signed-off-by: Chris Sampson <chris.sampson82@gmail.com>

exceptionfactory requested a review from ChrisSamo632 July 28, 2023 17:19

ChrisSamo632 requested changes Jul 30, 2023

View reviewed changes

This comment was marked as duplicate.

Sign in to view

dan-s1 closed this Aug 2, 2023

dan-s1 reopened this Aug 2, 2023

ChrisSamo632 reviewed Aug 2, 2023

View reviewed changes

...ssors/src/test/java/org/apache/nifi/processors/elasticsearch/PutElasticsearchRecordTest.java Outdated Show resolved Hide resolved

ChrisSamo632 requested changes Aug 2, 2023

View reviewed changes

ChrisSamo632 changed the title ~~NIFI-11778 Initial check in of refactored Groovy tests in nifi-elasticsearch-restapi-processors to Java (and JUnit 5)~~ NIFI-11778 refactored Groovy tests in nifi-elasticsearch-restapi-processors to Java (and JUnit 5) Aug 4, 2023

ChrisSamo632 reviewed Aug 7, 2023

View reviewed changes

ChrisSamo632 reviewed Aug 24, 2023

View reviewed changes

dan-s1 added 6 commits August 28, 2023 18:59

NIFI-11778 Initial check in of refactored Groovy tests in nifi-elasti…

93ab7f2

…csearch-restapi-processors to Java (and JUnit 5)

NIFI-11778 Attempt to fix unit tests which are failing.

e072368

NIFI-11778 Refactor based on PR review.

9d2c8e7

NIFI-11778 Refactored testPagination.

167533c

NIFI-11778 Made further changes per PR review.

27a254e

NIFI-11778 Made latest changes to simplify testing of SearchElasticse…

71c49d3

…archTest and PaginatedJsonQueryElasticsearchTest and incorporated the changes from apache#7441.

dan-s1 force-pushed the NIFI-11778 branch from a0db721 to 71c49d3 Compare August 28, 2023 19:01

ChrisSamo632 approved these changes Aug 28, 2023

View reviewed changes

asfgit closed this in 8c04526 Aug 29, 2023

asfgit pushed a commit that referenced this pull request Aug 29, 2023

NIFI-11778 refactored Groovy tests in nifi-elasticsearch-restapi-proc…

78c8f69

…essors to Java (and JUnit 5) This closes #7537 Signed-off-by: Chris Sampson <chris.sampson82@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NIFI-11778 refactored Groovy tests in nifi-elasticsearch-restapi-processors to Java (and JUnit 5) #7537

NIFI-11778 refactored Groovy tests in nifi-elasticsearch-restapi-processors to Java (and JUnit 5) #7537

dan-s1 commented Jul 28, 2023 •

edited by ChrisSamo632

exceptionfactory commented Jul 28, 2023

dan-s1 commented Jul 28, 2023 •

edited

ChrisSamo632 commented Jul 29, 2023

ChrisSamo632 left a comment

dan-s1 commented Aug 2, 2023 •

edited

dan-s1 commented Aug 2, 2023

This comment was marked as duplicate.

dan-s1 commented Aug 2, 2023 •

edited

ChrisSamo632 commented Aug 2, 2023

exceptionfactory commented Aug 2, 2023

ChrisSamo632 left a comment

ChrisSamo632 left a comment

dan-s1 commented Aug 22, 2023

ChrisSamo632 commented Aug 23, 2023

dan-s1 commented Aug 23, 2023 •

edited

ChrisSamo632 commented Aug 23, 2023

dan-s1 commented Aug 23, 2023 •

edited

exceptionfactory commented Aug 23, 2023

ChrisSamo632 left a comment

dan-s1 commented Aug 25, 2023 •

edited

dan-s1 commented Aug 25, 2023

dan-s1 commented Aug 28, 2023 •

edited

ChrisSamo632 commented Aug 28, 2023

dan-s1 commented Aug 28, 2023

exceptionfactory commented Aug 28, 2023

dan-s1 commented Aug 28, 2023

ChrisSamo632 left a comment

NIFI-11778 refactored Groovy tests in nifi-elasticsearch-restapi-processors to Java (and JUnit 5) #7537

NIFI-11778 refactored Groovy tests in nifi-elasticsearch-restapi-processors to Java (and JUnit 5) #7537

Conversation

dan-s1 commented Jul 28, 2023 • edited by ChrisSamo632

Summary

Tracking

Issue Tracking

Pull Request Tracking

Pull Request Formatting

Verification

Build

Licensing

Documentation

exceptionfactory commented Jul 28, 2023

dan-s1 commented Jul 28, 2023 • edited

ChrisSamo632 commented Jul 29, 2023

ChrisSamo632 left a comment

Choose a reason for hiding this comment

dan-s1 commented Aug 2, 2023 • edited

dan-s1 commented Aug 2, 2023

This comment was marked as duplicate.

dan-s1 commented Aug 2, 2023 • edited

ChrisSamo632 commented Aug 2, 2023

exceptionfactory commented Aug 2, 2023

ChrisSamo632 left a comment

Choose a reason for hiding this comment

ChrisSamo632 left a comment

Choose a reason for hiding this comment

dan-s1 commented Aug 22, 2023

ChrisSamo632 commented Aug 23, 2023

dan-s1 commented Aug 23, 2023 • edited

ChrisSamo632 commented Aug 23, 2023

dan-s1 commented Aug 23, 2023 • edited

exceptionfactory commented Aug 23, 2023

ChrisSamo632 left a comment

Choose a reason for hiding this comment

dan-s1 commented Aug 25, 2023 • edited

dan-s1 commented Aug 25, 2023

dan-s1 commented Aug 28, 2023 • edited

ChrisSamo632 commented Aug 28, 2023

dan-s1 commented Aug 28, 2023

exceptionfactory commented Aug 28, 2023

dan-s1 commented Aug 28, 2023

ChrisSamo632 left a comment

Choose a reason for hiding this comment

dan-s1 commented Jul 28, 2023 •

edited by ChrisSamo632

dan-s1 commented Jul 28, 2023 •

edited

dan-s1 commented Aug 2, 2023 •

edited

dan-s1 commented Aug 2, 2023 •

edited

dan-s1 commented Aug 23, 2023 •

edited

dan-s1 commented Aug 23, 2023 •

edited

dan-s1 commented Aug 25, 2023 •

edited

dan-s1 commented Aug 28, 2023 •

edited