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

Added Oracle test using a fork of testcontainers. #95

Merged
merged 2 commits into from
Jun 4, 2017

Conversation

npetzall
Copy link
Member

  • Using fork of TestContainers that allows assume on docker and sideloaded jdbcdriver jars.
  • Which means if docker doesn't exist or drivers are missing the test is ignored.
  • Added a TESTING.md which should be updated as we go along.
  • Minor restructuring in test/resources

@bsideup
Copy link

bsideup commented May 29, 2017

Hello.

TestContainers' co-maintainer is here. May I ask you not to use the fork? If TestContainers doesn't work for you, we're happy to adjust it for your use-case.
FYI Zipkin uses TestContainers and also supports fallback when Docker is not present, you can check it here:
https://github.com/openzipkin/zipkin/blob/master/zipkin-storage/elasticsearch-http/src/test/java/zipkin/storage/elasticsearch/http/integration/LazyElasticsearchHttpStorage.java

TC is being actively developed, �and SchemaSpy would benefit from future features, but a fork will lock it to the specific version.

Best Regards,
Sergei

@npetzall
Copy link
Member Author

npetzall commented May 29, 2017

@bsideup
testcontainers/testcontainers-java#343
I had a workaround with a wrapper rule. But I felt our codebase shouldn't be about testcontainers.
TestContainers, need a new class vs. fork two more arguments to a one-line.

testcontainers/testcontainers-java#344
This is more of a requirement.

None of the issues were of interest so, I would suspect that "adjust it for your use-case" has been proven to be un-true.

As the forker, I feel that your assuming that I won't keep it up-to-date, which I won't as long as the fork fills my needs.

@npetzall
Copy link
Member Author

From testcontainers channel: I assume there hasn't been any coding involved for decoupling TestContainers from JUnit yet?
So basically implementing this issue:
testcontainers/testcontainers-java#87 And the reason might be, that it's currently working for all of us, so this seems like a bit of yak shaving? 😉

@rafalkasa
Copy link
Member

@npetzall before I will merge you PR could you verify and resolve the conflicts ? Thank you very much.

* Using fork of TestContainers that allows assume on docker and sideloaded jdbcdriver jars.
* Which means if docker doesn't exist or drivers are missing the test is ignored.
* Added a TESTING.md which should be updated as we go along.
* Minor restructuring in test/resources
* Added additional dumpAll to ResultSetDumper
@npetzall
Copy link
Member Author

@rafalkasa Done

@ClassRule
public static OracleContainer oracleContainer = new OracleContainer<>()
.assumeDocker()
.withDrivers(driverPath);
Copy link
Member

Choose a reason for hiding this comment

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

@npetzall could you do something to not throw the exception when driverPath doesn't exist ?
In my opinion should be the possibility to skip integration test when some condition are not passed

Copy link
Member

Choose a reason for hiding this comment

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

Now I'm receiving the exception as below:

[ERROR] org.schemaspy.testcontainer.OracleIT  Time elapsed: 0.04 s  <<< ERROR!
java.lang.ExceptionInInitializerError
	at org.schemaspy.testcontainer.OracleIT.<clinit>(OracleIT.java:66)

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm stumped.
I've tested
docker on, missing dir
docker on, missing lib
docker off, lib exist
docker off, missing dir
docker off, missing lib

They all result in test ignored.

Copy link
Member Author

Choose a reason for hiding this comment

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

Are you sure you've pulled the latest changes. I only ask since travis-ci, marked the test as skipped and not failed.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm soon on windows and will test it again.

Copy link
Member

Choose a reason for hiding this comment

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

@npetzall you need to notice that I only go to the project folder and run mvnw verify and after this issue occured

Copy link
Member Author

@npetzall npetzall May 31, 2017

Choose a reason for hiding this comment

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

@rafalkasa Verified, know what's wrong. I'm on it

@npetzall
Copy link
Member Author

Strange, I'll check it to night. Only tested happy path after conflict resolution. That was sloppy.

@rafalkasa
Copy link
Member

My duty is check the thing twice before the merge with master 😄

@npetzall
Copy link
Member Author

Do you have more of the stacktrace?

@npetzall
Copy link
Member Author

Since it's docker related and I know your on Windows I might have some problem recreating the error

@rafalkasa
Copy link
Member

unfortunately not, this is only what appear :-( I don't have oracle driver and folder ext-lib/ maybe this is the issue

@npetzall
Copy link
Member Author

npetzall commented Jun 1, 2017

@rafalkasa I've fixed an Issue. Should work better now.

@rafalkasa
Copy link
Member

I can't test the changes because, I will don't have access to the computer to the end of the week. Maybe on Sunday I will have chance to look and verify

@npetzall
Copy link
Member Author

npetzall commented Jun 2, 2017

Got it, no worries, i'll work on something else during the wait.

@rafalkasa
Copy link
Member

@npetzall everything is working now after your last commit 😄

I reviewed your conversation related to the changes in testcontainer for java, as I understand we are using your forked testcontainer version instead original because of some limitation in existing implementation.

What do you think ? Could you maintain your testcontainer-java and update after original project releases ?

Maybe exist possibility to extend the testcontainer-java code instead fork the code and making changes inside. I'm thinking about built our own solution on top testcontainer-java and use original testcontainer as reference. Something like proxy api ? I don't know the details maybe you can explain more if it's possible ?

@rafalkasa rafalkasa merged commit ce88443 into schemaspy:master Jun 4, 2017
@npetzall
Copy link
Member Author

npetzall commented Jun 4, 2017

@rafalkasa
I'm looking into creating a PR for 343 that they can accept.

As for the driver loading issue 344. I'm gonna check and see if I can get it working by having maven adding it to classpath during integration test phase (not be able to run it from intellij, I'm guessing)

So for the junit module for testcontainers I'm gonna add so that it ignores if jdbc-driver is missing.

@npetzall
Copy link
Member Author

npetzall commented Jun 5, 2017

@rafalkasa we might be able to retire the usage of the fork if 343 is fixed. Was able to get failsafe working with additionalclasspathelement. Seems there was a magic setting missing.

@npetzall npetzall deleted the oracle_testcontainer branch November 15, 2017 23:31
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.

None yet

3 participants