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

[4Bmcyc2U] Fix ignored S3 tests #241

Merged
merged 3 commits into from
Nov 29, 2022
Merged

[4Bmcyc2U] Fix ignored S3 tests #241

merged 3 commits into from
Nov 29, 2022

Conversation

vga91
Copy link
Collaborator

@vga91 vga91 commented Nov 17, 2022

Fix ignored S3 tests.

Now we use a localstack container, as done in extended, in LoadS3Test.

  • Moved S3Container from apoc-extended (used in LoadS3Test) to test-utils. See related pr.
  • Added common S3BaseTest
  • Removed unused testImplementation group: 'com.amazonaws' from common/build.gradle
  • Removed envVar assumptions

S3Container changes:

  • Removed some unused code from the old S3Container
  • Simplified assertion handling.
    Instead of move the uploaded files from a S3 location to a temp directory and then compare the files with TestUtil.readFileToString, now the comparison is executed without moving the file directly with IOUtils.toString(s3File)

Other changes:

  • Updated pretty old LocalStack image version, from 0.8.0 to 1.2.0
  • created common GraphMLUtils.java
  • create FileUtils.assertStreamEquals (common to ExportJsonTest and ExportJsonS3Test)

Copy link
Contributor

@AzuObs AzuObs left a comment

Choose a reason for hiding this comment

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

Great work overall 🎉

My only comment is about the S3PerformanceTest which is taking 51 minutes to run. This is very long for a single test and I think a reasonable maximum should be closer to 30 seconds.

Screenshot 2022-11-22 at 11 15 38

@vga91
Copy link
Collaborator Author

vga91 commented Nov 28, 2022

I changed the S3PerformanceTest (now it takes about 1 min) with a smaller dataset created via some movies.cypher instead of a lot of executeTranctionally(stringQuery)

Copy link
Contributor

@AzuObs AzuObs left a comment

Choose a reason for hiding this comment

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

It makes a lot of sense to use the movies dataset, and 1 minute is fine for now.

Thanks a lot for the hard work. Approved 🎉

@vga91 vga91 merged commit 52a2b53 into dev Nov 29, 2022
@vga91 vga91 deleted the fix_ignored_s3_test branch November 29, 2022 11:52
vga91 added a commit to vga91/neo4j-apoc-procedures that referenced this pull request Dec 22, 2022
* [4Bmcyc2U] Fix ignored S3 tests
* fix tests
* lightened ExportS3PerformanceTest
vga91 added a commit to vga91/neo4j-apoc-procedures that referenced this pull request Dec 22, 2022
* [4Bmcyc2U] Fix ignored S3 tests
* fix tests
* lightened ExportS3PerformanceTest
vga91 added a commit to neo4j-contrib/neo4j-apoc-procedures that referenced this pull request Jan 3, 2023
* [4Bmcyc2U] Fix ignored S3 tests
* fix tests
* lightened ExportS3PerformanceTest
vga91 added a commit to neo4j-contrib/neo4j-apoc-procedures that referenced this pull request Jan 3, 2023
* [4Bmcyc2U] Fix ignored S3 tests
* fix tests
* lightened ExportS3PerformanceTest
vga91 added a commit to neo4j-contrib/neo4j-apoc-procedures that referenced this pull request Jan 4, 2023
* [4Bmcyc2U] Fix ignored S3 tests
* fix tests
* lightened ExportS3PerformanceTest
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants