akgrant43 commented 3 years ago

This PR makes two changes for Pharo 9 compatibility:

Change parameter type from ExternalAddress to void * to prevent 'object not pinned' errors.
Pharo 9 TFFI assumes functions with String return type are UTF8 and decodes

Changes:

Change the parameter type in:

SQLite3StatementExternalObject class>>finalizeResourceData: SQLite3DatabaseExternalObject class>>finalizeResourceData:
Modify SQLite3Library>>utf8StringToPharo: to not decode the UTF8 in Pharo 9 and later.

codecov[bot] commented 3 years ago

Codecov Report

Merging #41 (487693f) into master (7614fe8) will decrease coverage by 0.90%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master      #41      +/-   ##
==========================================
- Coverage   77.63%   76.72%   -0.91%     
==========================================
  Files         101      100       -1     
  Lines        4086     4009      -77     
==========================================
- Hits         3172     3076      -96     
- Misses        914      933      +19

Impacted Files	Coverage Δ
src/SQLite3-Core/SQLite3Library.class.st	`64.38% <ø> (-1.16%)`	:arrow_down:
...QLite3-Core/SQLite3DatabaseExternalObject.class.st	`100.00% <100.00%> (ø)`
src/SQLite3-Core/SQLite3PreparedStatement.class.st	`100.00% <100.00%> (ø)`
...Lite3-Core/SQLite3StatementExternalObject.class.st	`57.14% <100.00%> (ø)`
src/SQLite3-Core/SQLite3Row.class.st	`59.13% <0.00%> (-6.46%)`	:arrow_down:
src/SQLite3-Core/SQLite3BaseConnection.class.st	`91.39% <0.00%> (-3.28%)`	:arrow_down:
src/SQLite3-Glorp/SQLite3Driver.class.st

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 7614fe8...487693f. Read the comment docs.

akgrant43 commented 3 years ago

The Glorp test failures are pre-existing, but not consistent.

For example, in Pharo 8 (without this PR applied) GlorpDictionaryMappingTest>>#testDeleteSimpleTypeOneToMany will pass when stepping through the Glorp code, but will often fail if run normally. Enabling logging shows the following sequence of statements for PUBLISHER_TITLE (the primary table that is modified):

CREATE TABLE PUBLISHER_TITLE (PUBLISHER_ID int  NULL ,POSITION int  NULL ,TITLE text  NULL , CONSTRAINT PUBLISHER__TO_GR_PUBLISH_REF1 FOREIGN KEY (PUBLISHER_ID) REFERENCES GR_PUBLISHER (ID))

INSERT INTO PUBLISHER_TITLE (PUBLISHER_ID,POSITION,TITLE)  VALUES (?,?,?)  #(1 3 'Metadata-Based Persistence for Dummies')
INSERT INTO PUBLISHER_TITLE (PUBLISHER_ID,POSITION,TITLE)  VALUES (?,?,?)  #(2 1 'A book')
INSERT INTO PUBLISHER_TITLE (PUBLISHER_ID,POSITION,TITLE)  VALUES (?,?,?)  #(1 1 'Mastering ENVY/Developer')
INSERT INTO PUBLISHER_TITLE (PUBLISHER_ID,POSITION,TITLE)  VALUES (?,?,?)  #(1 2 'Principia Mathematica')

INSERT INTO PUBLISHER_TITLE (PUBLISHER_ID,POSITION,TITLE)  VALUES (?,?,?)  #(1 2 'Principia Mathematica')
DELETE FROM PUBLISHER_TITLE WHERE PUBLISHER_ID = ? AND POSITION = ? AND TITLE = ?  #(1 2 'Mastering ENVY/Developer')
DELETE FROM PUBLISHER_TITLE WHERE PUBLISHER_ID = ? AND POSITION = ? AND TITLE = ?  #(1 3 'Principia Mathematica')

This is clearly wrong as it is attempting to delete 'Principa Mathematica' with (publisher_id, position) pair (1 3), when the entry was created as (1 2). Similarly, 'Mastering ENVY/Developer' is created with (1 1), but attempts to delete with (1 2)

Note that the SQL statements can differ between runs when failing.

gcotelli commented 3 years ago

The Glorp test failures are pre-existing, but not consistent.

For example, in Pharo 8 (without this PR applied) GlorpDictionaryMappingTest>>#testDeleteSimpleTypeOneToMany will pass when stepping through the Glorp code, but will often fail if run normally. Enabling logging shows the following sequence of statements for PUBLISHER_TITLE (the primary table that is modified):
CREATE TABLE PUBLISHER_TITLE (PUBLISHER_ID int  NULL ,POSITION int  NULL ,TITLE text  NULL , CONSTRAINT PUBLISHER__TO_GR_PUBLISH_REF1 FOREIGN KEY (PUBLISHER_ID) REFERENCES GR_PUBLISHER (ID))

INSERT INTO PUBLISHER_TITLE (PUBLISHER_ID,POSITION,TITLE)  VALUES (?,?,?)  #(1 3 'Metadata-Based Persistence for Dummies')
INSERT INTO PUBLISHER_TITLE (PUBLISHER_ID,POSITION,TITLE)  VALUES (?,?,?)  #(2 1 'A book')
INSERT INTO PUBLISHER_TITLE (PUBLISHER_ID,POSITION,TITLE)  VALUES (?,?,?)  #(1 1 'Mastering ENVY/Developer')
INSERT INTO PUBLISHER_TITLE (PUBLISHER_ID,POSITION,TITLE)  VALUES (?,?,?)  #(1 2 'Principia Mathematica')

INSERT INTO PUBLISHER_TITLE (PUBLISHER_ID,POSITION,TITLE)  VALUES (?,?,?)  #(1 2 'Principia Mathematica')
DELETE FROM PUBLISHER_TITLE WHERE PUBLISHER_ID = ? AND POSITION = ? AND TITLE = ?  #(1 2 'Mastering ENVY/Developer')
DELETE FROM PUBLISHER_TITLE WHERE PUBLISHER_ID = ? AND POSITION = ? AND TITLE = ?  #(1 3 'Principia Mathematica')
This is clearly wrong as it is attempting to delete 'Principa Mathematica' with (publisher_id, position) pair (1 3), when the entry was created as (1 2). Similarly, 'Mastering ENVY/Developer' is created with (1 1), but attempts to delete with (1 2)

Note that the SQL statements can differ between runs when failing.

Certainly, the changes don't look like something that will make this specific test fail.

akgrant43 commented 3 years ago

Hmm, not sure why the Unit Tests are failing here. The load and tests run successfully on my local machine. Investigating...

akgrant43 commented 3 years ago

Help! :-)

This looks like a CI issue to me. I can run smalltalkCI locally and all tests pass for both Pharo 8 and 9. I've refactored the baseline as Gabriel kindly suggested, and smalltalkCI still passes locally.

The errors kind of look like it is using the old baseline with the new repository code, like the image was saved with the baseline in it.

Any suggestions?

gcotelli commented 3 years ago

I will check the code and let you know if I find anything. Taking a quick look seems to be ok so I don't understand why it fails.

akgrant43 commented 3 years ago

I will check the code and let you know if I find anything. Taking a quick look seems to be ok so I don't understand why it fails.

Thanks!

akgrant43 commented 3 years ago

Hi Garbriel, Thanks very much for fixing and merging the PR! What was the significant change that made the baseline load correctly? From what I can see, it looks like the only thing you changed was specifying #group:with: immediately after #package: (the logging obviously shouldn't have any effect, and grouping pharo 7 & 8 together in #versionSpecificBaseline: shouldn't matter).

gcotelli commented 3 years ago

I've removed blessing: #baseline and changed the dependency on SQLite3-Core-Tests package to be against the Core group instead of a package. I think this last change was the one that make the project load.

The rest are cosmetic changes.

akgrant43 commented 3 years ago

I've removed blessing: #baseline and changed the dependency on SQLite3-Core-Tests package to be against the Core group instead of a package. I think this last change was the one that make the project load.

The rest are cosmetic changes.

Weird. But thanks for the explanation!

pharo-rdbms / Pharo-SQLite3

Pharo 9 TFFI updates #41

Codecov Report