@MMatten I don't quite understand why changes need to be made to AbstractDbEnvironment. Is it possible to push them down to TeradataEnvironment?

Hmm there's a lot of changes in this pull request. @MMatten I'd prefer to split it into at least two parts:

• just 1fc6037 (but without the debug statements)
• the savepoints support (which seems a lot more invasive and needs more discussion)

I did initially change AbstractDbEnvironment to allow createStatementWithBoundFixtureSymbols to be overridden and then called either setObject or setNull in TeradataEnviroment. The Teradata driver seems to accept java.sql.Types.NULL as the type regardless of the actual type in the DB whereas other drivers wouldn't (neither would they take Types.VARCHAR and automatically convert).

The Teradata createStatementWithBoundFixtureSymbols was almost identical to that of the super type so I though of eliminating (almost) duplicated code but at the code of changing AbstractDbEnvironment and introducing the supportsSetObjectNull() and getJavaClassSqlType(). I can revert this easily. Let me know your thoughts.

I did initially change AbstractDbEnvironment to allow createStatementWithBoundFixtureSymbols to be overridden and then called either setObject or setNull in TeradataEnviroment. The Teradata driver seems to accept java.sql.Types.NULL as the type regardless of the actual type in the DB whereas other drivers wouldn't (neither would they take Types.VARCHAR and automatically convert).

The Teradata createStatementWithBoundFixtureSymbols was almost identical to that of the super type so I though of eliminating (almost) duplicated code but at the code of changing AbstractDbEnvironment and introducing the supportsSetObjectNull() and getJavaClassSqlType(). I can revert this easily. Let me know your thoughts.

Maybe that set NULL support is a good candidate to handle in separate PR (will reduce the complexity of this one).

• First question is: can we figure out a relatively simple and portable support for that.
• Maybe extract setObject method in db environment(s).

@benilovj Jake, I've realised1fc6037 doesn't build. The change was good for Teradata but breaks other DB types.

I'm not sure how to proceed. Teradata can't work with savepoints and can't work with setObject with a null object ref.

What if I revert the changes to DBEnvironment/AbstractDbEnvironment and make createStatementWithBoundFixtureSymbols overrideable in AbstractDbEnvironment call setNull in the Teradata variant?

I'd have to leave my changes in StatementExecution and SymbolUtil etc though. Otherwise I'll have to wait for you guys to make some core changes.

Heyelp!

@MMatten we've started making some core changes that will allow you to simplify your pull request quite considerably. Bear with us...

@MMatten so we've removed handling of savepoints now (#309).

So, to move this change forward:

• could you please remove the savepoint-handling in your code
• as @javornikolov suggested, the null handling is best done as a subsequent pull request, straight after this one would be merged.

I think those two points should mean that you wouldn't need any core changes at all...

@benilovj,

To clarify:

For this pull request I'll: -

• Abandon my changes to StatementExecution.
• Abandon the proposed supportsSavepoints method on AbstractDbEnvironment.
• Abandon the proposed supportsSetObjectNull method on AbstractDbEnvironment.
• Abandon the other changes to createStatementWithBoundFixtureSymbols.
• Send you a pull request for the remainder (revised Teradata tests mainly, with a few other fixes to TeradataEnvironment).

This will obviously result in the Teradata code not being usable still and it won't pass tests on in my dev env. But you'll merge this.

I'll then work on a new PR for the null handing to make it all work.

Is this what you are expecting?

yes that sounds like the best way to proceed.

How many tests are we expecting to be failing without the null handling?

Not sure off the top of my head. Pretty much anything involving a null!

Tests updated/refined and whitespace-only changes reverted with fixes applied.

Do I need to disable the TD regression suite, or do you think the 4GB TD setup is ok (even if not the latest version of TD Express that's available)?

Do I need to disabled the TD regression suite, or do you think the 4GB TD setup is ok (even if not the latest version of TD Express that's avaiable)?

Main question here is - what would be impact on other people which would like to contribute?

So far I managed to find how to skip some tests of specific db: gradlew check -x :dbfit-java:teradata:test.

I think we can set @Ignore on the Teradata acceptance tests for now. And later we can think of some way for easier way to skip/force the execution of acceptance tests for specified tests.

If people are introducing core changes then they really should run all of the suites (as already mentioned in the docs), otherwise they can run the tests for their adapter changes (again, in the docs).

As it looks like there could be more adapters coming down the line (Sybase, Netezza [although no Linux dev version] and may be even Informix) I think maintaining a dev machine running all of these will be a tall order.

So perhaps the current instructions in the docs are spot on.

As far as the VM is concerned, the Oracle and DB2 installations are optional, according to the CONTRIBUTING doc, but the tests are enabled - may be these shouldn't enabled by default either then.

As far as the VM is concerned, the Oracle and DB2 installations are optional, according to the CONTRIBUTING doc, but the tests are enabled - may be these shouldn't enabled by default either then.

I also wondered about these - the situation is not ideal with them too. Though they're a bit easier to setup and are able to run inside the VM.

Maybe the way to go would be to allow overriding what tests to run or not in some config. file.

Yes, I had the same thought; a config file or may be some !defines on a parent FitNesse page to !include or not the actual tests, or something even crazier.

I'll fix up the points you've just spotted.

Done. I've also updated TERADATA.md to specify the transaction mode as TERA else the stored procs will fail, if created using an ANSI mode connection, when called as the current connections in the tests are TERA mode.

OK, thanks!
Maybe this is already good enough to merge, @benilovj?

@javornikolov yes I think we're there for this PR.

What I'll do this evening is squash most commits and peel off a few, to get a cleaner commit log.

Great. I can then create some new issues for the NULL handling, redundant tests (i.e. to hook into the core tests), the remaining reformatting (that I recently reverted), and to execute the tests in both TERA and ANSI modes.

Ok, it took some effort but I think I finally have a mergeable, rebased, consolidated branch with just 1 commit which should reflect the changes here exactly:

https://github.com/dbfit/dbfit/compare/pmsoftware-master

Now, one thing I noticed while doing this was that the proposed changes still don't reuse the common "core" tests. This adds a lot more (potentially duplicated) tests, and I'm not sure that there's a good reason for that anymore, since we added the Execute DDL fixture. Is there anything stopping us from reusing the core tests here?

oh wait - did we decide to do the test reorg on another pull request?

oh wait - did we decide to do the test reorg on another pull request?

Yes. When the NULL support is sorted out - it should be simpler to just pull the Core tests here.

Ok. Good news. Thanks for all the help on this!

Next up then is addresses the NULL handing and re-enabling the tests including NULLs.

Regarding using the CoreTests - do you this should be in the same PR as the NULLs work or keep it separate?

Ok. Good news. Thanks for all the help on this!

Thanks for all the hard work!

Regarding using the CoreTests - do you this should be in the same PR as the NULLs work or keep it separate?

Probably best kept separate - will make it easier to review.

Since #316 is merged, closing.