Skip to content

Conversation

@JoshRosen
Copy link
Contributor

This PR adds end-to-end integration tests that run against a real Redshift cluster.

I moved the build definition from .sbt to .scala format in order to use SBT's standard IntegrationTests configuration.

I configured the Travis build to store Redshift credentials using encrypted environment variables. To minimize security risks, Travis does not allow pull requests from third-party forks to run tests using these credentials.

@codecov-io
Copy link

Current coverage is 85.90%

Merging #41 into master will not affect coverage as of 6dff0a2

@@            master    #41   diff @@
=====================================
  Files            9      9       
  Stmts          305    305       
  Branches        70     70       
  Methods          0      0       
=====================================
  Hit            262    262       
  Partial          0      0       
  Missed          43     43       

Review entire Coverage Diff as of 6dff0a2

Powered by Codecov. Updated on successful CI builds.

@JoshRosen
Copy link
Contributor Author

Alright, the basic test is now passing (fixed the non-deterministic failure issue), so I'm now going to focus on enabling more tests.

.travis.yml Outdated
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wonder if I should purposely give these different names than the ones expected by the Amazon SDKs; this might be necessary in order to be able to test credential mechanisms (e.g. for writing a regression test for #32)

@JoshRosen
Copy link
Contributor Author

All tests are now enabled, but some seem to fail due to problems in the tests themselves (due to not using a HiveContext) or due to bugs.

@JoshRosen JoshRosen changed the title [WIP] Add integration tests which run against a real Redshift cluster Add integration tests which run against a real Redshift cluster Aug 19, 2015
@JoshRosen JoshRosen changed the title Add integration tests which run against a real Redshift cluster [WIP] Add integration tests which run against a real Redshift cluster Aug 19, 2015
@JoshRosen
Copy link
Contributor Author

This is now ready for review. Let's not commit until #42 goes in and until I have time for one final pass. This isn't all of the tests that I want to add but it's a reasonable first cut; I'll add more tests and fix more potential bugs in smaller followups.

.travis.yml Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: is there a reason to not test style and other fast stuff first?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted the tests to still be able to run even if style checks failed. I figured this wasn't a huge deal here compared to what we do in Spark because the test time is really fast.

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: newline

@marmbrus
Copy link
Contributor

This is awesome! LGTM.

@JoshRosen
Copy link
Contributor Author

This was rebased on top of #32, but I'm going to try to back out of that rebase so that we can write a proper regression test for that issue.

@JoshRosen JoshRosen changed the title [WIP] Add integration tests which run against a real Redshift cluster Add integration tests which run against a real Redshift cluster Aug 20, 2015
@JoshRosen JoshRosen changed the title Add integration tests which run against a real Redshift cluster Add integration tests that run against a real Redshift cluster Aug 20, 2015
@JoshRosen
Copy link
Contributor Author

Alright, I have now backed out all non-test-related changes and have addressed the review comments, so I'm going to merge this now and will continue to iterate in followups.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants