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

Enable oracle tests #5683

Merged
merged 40 commits into from
Dec 5, 2019
Merged

Enable oracle tests #5683

merged 40 commits into from
Dec 5, 2019

Conversation

koppor
Copy link
Member

@koppor koppor commented Nov 28, 2019

Enable test on Oracle XE. Fixes JabRef#151.

Oracle XE encrypted available at files.jabref.org. We have the password stored at koppor's KeePass and in the environment variable during the build. Thus, it can be decrypted during the test, but (hopefully) not by other persons.

  • Remove obsolete ";" at SQL statement (ignored by non-Oracle-DB-systems)
  • create prepared statement
  • reformat code

@koppor koppor force-pushed the enable-oracle-tests branch 7 times, most recently from 0d89196 to 7a8f178 Compare November 28, 2019 22:03
@koppor koppor force-pushed the enable-oracle-tests branch from 49f4521 to eb3ac7b Compare November 30, 2019 13:33
@koppor
Copy link
Member Author

koppor commented Dec 1, 2019

Login into a local docker image works (after creating the jabref user)

$ docker exec -it  oracle-xe bash -c "source /home/oracle/.bashrc; /bin/bash -c '$ORACLE_HOME/bin/sqlplus jabref/jabref@localhost/XEPDB1'"

Then both following statements work:

SELECT ENTRY.SHARED_ID, ENTRY.TYPE, ENTRY.VERSION, F.ENTRY_SHARED_ID, F.NAME, F.VALUE FROM ENTRY inner join FIELD F on ENTRY.SHARED_ID = F.ENTRY_SHARED_ID order by SHARED_ID;
SELECT "ENTRY"."SHARED_ID", "ENTRY"."TYPE", "ENTRY"."VERSION", F."ENTRY_SHARED_ID", F."NAME", F."VALUE" FROM "ENTRY" inner join "FIELD" F on "ENTRY"."SHARED_ID" = F."ENTRY_SHARED_ID" order by "SHARED_ID";

But not when using the oracleJDBC driver:

23:14:30.001 [main] ERROR org.jabref.logic.shared.DBMSProcessor - Executed >SELECT ENTRY.SHARED_ID, ENTRY.TYPE, ENTRY.VERSION, F.ENTRY_SHARED_ID, F.NAME, F.VALUE FROM ENTRY inner join FIELD F on ENTRY.SHARED_ID = F.ENTRY_SHARED_ID where SHARED_ID in (1) order by SHARED_ID;< -  
23:14:30.001 [main] ERROR org.jabref.logic.shared.DBMSProcessor - SQL Error - java.sql.SQLSyntaxErrorException: ORA-00933: SQL-Befehl wurde nicht korrekt beendet

	at oracle.jdbc.driver.T4CTTIoer11.processError(T4CTTIoer11.java:509)
	at oracle.jdbc.driver.T4CTTIoer11.processError(T4CTTIoer11.java:461)
	at oracle.jdbc.driver.T4C8Oall.processError(T4C8Oall.java:1104)
	at oracle.jdbc.driver.T4CTTIfun.receive(T4CTTIfun.java:550) 

The simplest test case probably is: org.jabref.logic.shared.DBMSProcessorTest#testGetSharedEntry

@koppor koppor force-pushed the enable-oracle-tests branch from a76d12b to 6918e7b Compare December 2, 2019 20:33
@koppor koppor marked this pull request as ready for review December 4, 2019 05:22
@koppor koppor added status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers and removed status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers labels Dec 4, 2019
Copy link
Member

@tobiasdiez tobiasdiez left a comment

Choose a reason for hiding this comment

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

The changes look good. However, now the build takes > 30 min, from originally 3-4 mins. Because of this, I would propose to extract the oracle tests to a separate workflow that runs regularly every week instead of at each commit.

query.append(" where ")
.append(escape("SHARED_ID")).append(" in (")
.append(idListAsString)
.append(")");
.append("?, ".repeat(sharedIDs.size() - 1))
Copy link
Member

Choose a reason for hiding this comment

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

The previous version was more readable in my opinion. What was the reason for this change?

Copy link
Member Author

Choose a reason for hiding this comment

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

Made id consistent with other parts of DBMSProcessor (saw it in the last pull requests going through. I especially reused the idea of #5659 (comment)).

@koppor koppor added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Dec 4, 2019
with:
path: ~/.gradle/wrapper
key: ${{ runner.os }}-gradle-${{ hashFiles('gradle/wrapper/gradle-wrapper.properties') }}
- name: Cache Oracle XE downloads
Copy link
Member

Choose a reason for hiding this comment

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

[warning]Cache size of ~2455 MB (2573776080 B) is over the 400MB limit, not saving cache.

I think github will not increase the cache size that dramatically in the near future. Thus, this cache can be removed (which reduces the build time by 2 mins - not much but better than nothing).

Copy link
Member

Choose a reason for hiding this comment

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

For caching, see the discussion here: actions/cache#6

Copy link
Member Author

Choose a reason for hiding this comment

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

Result of the discussion:

There may be another update soon to up the limit a little (~700MB or so), [...] We would still have the 2GB per repo limit until we integrate billing, so there would be some concern about thrashing the cache if the individual limit matched the per repo limit.

@koppor koppor merged commit d3ca1f6 into master Dec 5, 2019
@koppor koppor deleted the enable-oracle-tests branch December 5, 2019 20:26
Siedlerchr added a commit that referenced this pull request Dec 8, 2019
* upstream/master:
  Add throttle to AutosaveUIManager (#5680)
  Do not couple search position to sidebar width (#5716)
  fix springer fetcher tests
  Bump controlsfx from 11.0.0 to 11.0.1 (#5714)
  Add CHANGELOG.md entry for Oracle
  Enable oracle tests (#5683)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enable testing of Oracle on TravisCI
4 participants