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

Transaction tests assume database schema app #51

Closed
chengfang opened this issue May 25, 2022 · 4 comments · Fixed by #54
Closed

Transaction tests assume database schema app #51

chengfang opened this issue May 25, 2022 · 4 comments · Fixed by #54
Labels
accepted Has a specific meaning in TCK process - used for TCK challenge that was accepted challenge

Comments

@chengfang
Copy link
Contributor

chengfang commented May 25, 2022

The relevant specification version and section number(s): 2.1

The coordinates of the challenged test(s): com.ibm.jbatch.tck.tests.jslxml.TransactionTests

The exact TCK version: 2.1

The implementation being tested, including name and company: JBeret, by Red Hat, Inc.

A full description of why the test is invalid and what the correct behavior is believed to be:
Any supporting material; debug logs, test output, test logs, run scripts, etc.:

TransactionTests assumes all database tables are in APP schema. This is the default schema in derby. So when testing with Derby, it always works, with this schema name in some places, and without it in other places. But it will not work with any other db.

The ConnectionHelper class and RetryConnectionHelper class use app schema in all sql statements.

DDL files don't use schema.

Failed tests:

[ERROR] Failures:
[ERROR]   TransactionTests.testGlobalTranForcedExceptionCheckpointRestart:349 Expected value: Inventory=81 InitialCheckpoint=null OrderCount=18, but found value: FAILED
[ERROR]   TransactionTests.testGlobalTranForcedExceptionWithRollback:295 Expected value: Inventory=81 InitialCheckpoint=null OrderCount=18, but found value: FAILED
[ERROR]   TransactionTests.testGlobalTranLongDelayMixOfLongTimeoutStepsAndShortTimeoutSteps:477 Expected value: COMPLETED, but found value: FAILED
[ERROR]   TransactionTests.testGlobalTranLongDelayMixOfLongTimeoutStepsAndShortTimeoutStepsCustomCheckpointAlgorithm:550 Expected value: COMPLETED, but found value: FAILED
[ERROR]   TransactionTests.testGlobalTranNoDelayLongTimeout:416 Expected value: Inventory=0 InitialCheckpoint=null OrderCount=99, but found value: FAILED
[ERROR]   TransactionTests.testGlobalTranNoExceptions:243 Expected value: Inventory=0 InitialCheckpoint=null OrderCount=99, but found value: FAILED
[ERROR]   TransactionTests.testTranRollbackRetryProcessSkipProcess:146 Expected value: COMPLETED, but found value: FAILED
[ERROR]   TransactionTests.testTranRollbackRetryReadSkipRead:99 Expected value: COMPLETED, but found value: FAILED
[ERROR]   TransactionTests.testTranRollbackRetryWriteSkipWrite:193 Expected value: COMPLETED, but found value: FAILED

I suggest removing schema name from all sql statements, to make it easy to configure with other db products.

@scottkurz
Copy link
Contributor

@chengfang, agree it'd be a good idea to make a change to avoid and/or parameterize the schema/HLQ.

Let me ask: is it possible to work around this limitation by modifying the DDL?

We don't say much about the creation of the test database in the official documentation. Are you able to work around this limitation with the DB you're using and use the hard-coded 'APP' schema?

If so we could fix with a bit less urgency. Thanks.

@chengfang
Copy link
Contributor Author

@scottkurz It's not a blocker for JBeret and WildFly, as I managed to have a workaround with H2 (https://github.com/jberet/jberet-tck-porting/blob/master/src/main/java/org/jberet/tck/config/BatchTckConfigBean.java#L39).

If the TCK wants to keep the schema, then the TCK Guide will need to be updated with this information, which seems more change than just removing the schema name altogether. And the latter will make the TCK easier to work with in the future.

@scottkurz
Copy link
Contributor

According to the TCK process,we do have to address this challenge promptly one way or the other.

I propose we document the workaround and open an enhancement issue for later.

In more detail:

PROPOSED CHALLENGE HANDLING

  1. Add an explanation to the TCK Reference Guide doc referencing this original issue and explaining that the workaround is to create the TCK app tables under the 'APP' HLQ. (As part of this workaround, mention too the point that the provided DDLs are incomplete and need to be supplemented).
  2. Close this issue and label accepted.
  3. Open a new enhancement issue

Note: the TCK process does allow for resolving accepted challenges via workarounds.

PROPOSED FIX DETAILS

To address your points about the test assumptions, it seems like a solution here should allow for both a HLQ set from a system property AND the option of not using an HLQ altogether.

I also think the default behavior without new props should be back-compatible and translate into:
app.<table>

So we probably would need two properties: one to disable HLQ and one to set it to a non-default value.

This change would need to apply to the TCK runtime (ConnectionHelper, RetryConnectionHelper) and not something we could address only in the setup DDLs.

It would require some small amount of testing across each of Derby, non-Derby DBs.

THOUGHTS?

Even though the code change is small and simple it's not quite trivial, especially since this brings in the need to test against multiple DBs (this is part of the reason I went into detail on the proposed fix). I would rather not do it in the 2.1 "stream" if we can avoid it.

@chengfang, is this OK with you?

@chengfang
Copy link
Contributor Author

@scottkurz That works for me. I'm ok with not fixing it in 2.1 stream.

If we fix it in the next (2.2) stream, that means we will have leeway in the test design. How about completely removing the HLQ? As I understand it, having HLQ or not will not affect bath test or coverage one way or another.

See some other db files in platform TCK (no hlq used): https://github.com/chengfang/jakartaee-tck/blob/master/sql/derby/derby.ddl.jstl.sql

scottkurz added a commit to scottkurz/batch-tck that referenced this issue Jun 3, 2022
…dependence on Derby as database product

Signed-off-by: Scott Kurz <skurz@us.ibm.com>
scottkurz added a commit to scottkurz/batch-tck that referenced this issue Jun 3, 2022
…dependence on Derby as database product

Signed-off-by: Scott Kurz <skurz@us.ibm.com>
scottkurz added a commit that referenced this issue Jun 3, 2022
…-workaround-issue51

Add doc explaining workaround for issue #51 challenge - overdependenc…
@scottkurz scottkurz added the accepted Has a specific meaning in TCK process - used for TCK challenge that was accepted label Jun 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted Has a specific meaning in TCK process - used for TCK challenge that was accepted challenge
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants