Fix unit tests (#2876)

* Fix unit tests

- no longer assume CIF -> PDB residue numbers are stable
 (based on chain perception)
- normalize JSON output to ignore minor roundoff digits
- Fix CDJSON test for normalization / string comparisons

* Fix some CJSON memory issues discovered with Avogadro2
* Fix some compile issues on Windows

* Fixup workflows to cancel failing tmate after 60 minutes
  (Currently eating up a lot of time)
* Fix upload path to prevent actions from failing
* Make sure Cairo is installed on Linux for PNG tests
* Make sure to get the most recent packages with Linux dependencies

Signed-off-by: Geoff Hutchison <geoff.hutchison@gmail.com>

---------

Signed-off-by: Geoff Hutchison <geoff.hutchison@gmail.com>

.github/dependabot.yml

@@ -4,7 +4,8 @@ version: 2
 updates:
 
   - package-ecosystem: "github-actions"
-    directory: "/"
+    directories:
+      - "/.github/**/*"
     schedule:
       # Check for updates to GitHub Actions every weekday
       interval: "daily"

.github/workflows/build_bindings.yml

@@ -21,7 +21,7 @@ jobs:
           }
 
     steps:
-    - uses: actions/checkout@v4
+    - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2
 
     - name: Install Dependencies (Linux)
       if: runner.os == 'Linux'
@@ -46,7 +46,7 @@ jobs:
       working-directory: ${{ runner.workspace }}
 
     - name: Upload
-      uses: actions/upload-artifact@v4
+      uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1
       with:
         path: ${{ runner.workspace }}/openbabel-latest.tar.bz2
         name: openbabel.tar.bz2

.github/workflows/build_cmake.yml

@@ -5,6 +5,10 @@ on: [push, pull_request]
 env:
   BUILD_TYPE: Release
 
+concurrency:
+  group: ${{ github.workflow }}-${{ github.ref }}
+  cancel-in-progress: true
+
 jobs:
   build:
     name: ${{ matrix.config.name }}
@@ -41,17 +45,19 @@ jobs:
     - name: Check variables
       run: echo "${{runner.os}} ${{matrix.config.os}} ${{matrix.config.cc}}"
 
-    - uses: actions/checkout@v4
+    - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2
 
     - name: Install MSVC Command Prompt
       if: runner.os == 'Windows'
-      uses: ilammy/msvc-dev-cmd@v1
+      uses: ilammy/msvc-dev-cmd@0b201ec74fa43914dc39ae48a89fd1d8cb592756 # v1.13.0
       with:
         arch: x86
 
     - name: Install Dependencies (Linux)
       if: runner.os == 'Linux'
-      run: sudo apt-get -qq install ninja-build swig libeigen3-dev libboost-all-dev
+      run: |
+        sudo apt-get -qq update
+        sudo apt-get -qq install ninja-build swig libeigen3-dev libboost-all-dev libcairo2-dev
 
     - name: Install Dependencies (Windows)
       if: runner.os == 'Windows'
@@ -103,11 +109,12 @@ jobs:
 
     - name: Upload
       if: matrix.config.artifact != 0
-      uses: actions/upload-artifact@v4
+      uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1
       with:
-        path: ${{ runner.workspace }}/instdir/../${{ matrix.config.artifact }}
+        path: ${{ runner.workspace }}/${{ matrix.config.artifact }}
         name: ${{ matrix.config.artifact }}
 
     - name: Setup tmate debugging session
       if: ${{ failure() }}
-      uses: mxschmitt/action-tmate@v3
+      uses: mxschmitt/action-tmate@c0afd6f790e3a5564914980036ebf83216678101 # v3.23
+      timeout-minutes: 60

src/formats/gaussformat.cpp

@@ -801,7 +801,7 @@ namespace OpenBabel
                 tokenize(vs,buffer);
 
               }
-            if (CM5_q.size() == mol.NumAtoms() and
+            if (CM5_q.size() == mol.NumAtoms() &&
                 HPA_q.size() == mol.NumAtoms())
             {
                 Hirshfeld->AddPartialCharge(HPA_q);

src/formats/json/chemicaljsonformat.cpp

@@ -219,6 +219,7 @@ bool ChemicalJSONFormat::ReadMolecule(OBBase *pOb, OBConversion *pConv) {
   if (inRoot.HasMember("unitCell") && inRoot["unitCell"].IsObject()) {
     const rapidjson::Value &unitCell = inRoot["unitCell"];
     OBUnitCell *uc = new OBUnitCell();
+    bool ucUsed = false;
 
     // if spaceGroup is set use it
     if (unitCell.HasMember("spaceGroup") && unitCell["spaceGroup"].IsString()) {
@@ -241,6 +242,7 @@ bool ChemicalJSONFormat::ReadMolecule(OBBase *pOb, OBConversion *pConv) {
 
         uc->SetData(v1, v2, v3);
         pmol->SetData(uc);
+        ucUsed = true;
       }
     } else if (unitCell.HasMember("a") && unitCell["a"].IsDouble() &&
                unitCell.HasMember("b") && unitCell["b"].IsDouble() &&
@@ -258,6 +260,11 @@ bool ChemicalJSONFormat::ReadMolecule(OBBase *pOb, OBConversion *pConv) {
       uc->SetData(a, b, c, alpha, beta, gamma);
 
       pmol->SetData(uc);
+      ucUsed = true;
+    }
+
+    if (!ucUsed) {
+      delete uc;
     }
   }
 
@@ -321,8 +328,8 @@ bool ChemicalJSONFormat::WriteMolecule(OBBase *pOb, OBConversion *pConv) {
     // check for NMR shifts
     if (patom->HasData("NMR Isotropic Shift"))
       nmrShifts.PushBack(
-          rapidjson::StringRef(
-              patom->GetData("NMR Isotropic Shift")->GetValue().c_str()),
+          rapidjson::Value(
+              patom->GetData("NMR Isotropic Shift")->GetValue().c_str(), al),
           al);
   }
 
@@ -356,7 +363,7 @@ bool ChemicalJSONFormat::WriteMolecule(OBBase *pOb, OBConversion *pConv) {
   doc.AddMember("atoms", atoms, al);
 
   rapidjson::Value charges(rapidjson::kObjectType);
-  charges.AddMember(rapidjson::StringRef(chargeMethod.c_str()), partialCharges,
+  charges.AddMember(rapidjson::Value(chargeMethod.c_str(), al), partialCharges,
                     al);
   doc.AddMember("partialCharges", charges, al);
 
@@ -404,8 +411,9 @@ bool ChemicalJSONFormat::WriteMolecule(OBBase *pOb, OBConversion *pConv) {
 
       // get the space group
       if (uc->GetSpaceGroup() != nullptr) {
-        unitCell.AddMember("spaceGroup",
-                           rapidjson::StringRef(uc->GetSpaceGroupName().c_str()), al);
+        unitCell.AddMember(
+            "spaceGroup", rapidjson::Value(uc->GetSpaceGroupName().c_str(), al),
+            al);
       }
 
       doc.AddMember("unitCell", unitCell, al);
@@ -521,7 +529,7 @@ bool ChemicalJSONFormat::WriteMolecule(OBBase *pOb, OBConversion *pConv) {
   // other properties
 
   rapidjson::Value properties(rapidjson::kObjectType);
-  properties.AddMember("name", rapidjson::StringRef(pmol->GetTitle()), al);
+  properties.AddMember("name", rapidjson::Value(pmol->GetTitle(), al), al);
   properties.AddMember("totalCharge", pmol->GetTotalCharge(), al);
   properties.AddMember("totalSpinMultiplicity",
                        pmol->GetTotalSpinMultiplicity(), al);
@@ -529,7 +537,7 @@ bool ChemicalJSONFormat::WriteMolecule(OBBase *pOb, OBConversion *pConv) {
   // look for conformer energies
   if (pmol->HasData(OBGenericDataType::ConformerData)) {
     OBConformerData *cd =
-        (OBConformerData *) pmol->GetData(OBGenericDataType::ConformerData);
+        (OBConformerData *)pmol->GetData(OBGenericDataType::ConformerData);
     vector<double> energies = cd->GetEnergies();
     rapidjson::Value confEnergies(rapidjson::kArrayType);
     for (auto i = energies.begin(); i != energies.end(); ++i) {

src/formats/vaspformat.cpp

@@ -451,7 +451,7 @@ namespace OpenBabel {
             vector<vector3> vib;
             tokenize(vs, buffer);
             int freqnum = atoi(vs[0].c_str());
-            if (vs[1].size() == 1 and vs[1].compare("f") == 0) {
+            if (vs[1].size() == 1 && vs[1].compare("f") == 0) {
               // Real frequency
               Frequencies.push_back(atof(vs[7].c_str()));
             } else if (strstr(vs[1].c_str(), "f/i=")) {

test/cifspacegrouptest.cpp

@@ -274,7 +274,10 @@ void testPdbOccupancies()
   pdb = conv.WriteString(&mol);
 
   OB_ASSERT(pdb.find("HETATM    1 NA   UNL     1       0.325   0.000   4.425  0.36") != string::npos);
-  OB_ASSERT(pdb.find("HETATM   17  O   UNL     8       1.954   8.956   3.035  1.00") != string::npos);
+  // residue number from CIF depends on chain-perception heuristics; just check
+  // that atom 17 is an oxygen at the expected position and full occupancy
+  OB_ASSERT(pdb.find("HETATM   17  O   UNL") != string::npos);
+  OB_ASSERT(pdb.find("       1.954   8.956   3.035  1.00") != string::npos);
 
   OBMol mol_pdb;
   conv.SetInFormat("pdb");

test/testobconv_writers.py

@@ -669,6 +669,15 @@ Oc1ccccc1\tphenol
 """)
 
 # cdjson -- ChemDoodle JSON
+# Coordinates that come from --gen2D etc. are computed in double precision and
+# the last digit or two can differ across platforms / math libraries. Round any
+# long decimals down to a precision that compares reliably.
+_json_float_pat = re.compile(r"-?\d+\.\d{8,}")
+def _round_json_match(m):
+    return "%.10g" % float(m.group(0))
+def normalize_json_floats(content):
+    return _json_float_pat.sub(_round_json_match, content)
+
 class TestCDJSON(unittest.TestCase, WriteMixin):
     fmt = "cdjson"
     maxDiff = None
@@ -743,7 +752,7 @@ class TestCDJSON(unittest.TestCase, WriteMixin):
       ]
     }
   ]
-}""")
+}""", normalize=normalize_json_floats)
 
 ## # cdxml -- ChemDraw CDXML format
 ## XXX fails on an unpatched system
@@ -2754,6 +2763,10 @@ DrawText 40.0 81.9 "HO"
 """)
 
 # pcjson -- PubChem JSON
+# Uses normalize_json_floats (see TestCDJSON) to round long double-precision
+# decimals to a stable representation across platforms / math libraries.
+normalize_pcjson_floats = normalize_json_floats
+
 class TestPCJSON(unittest.TestCase, WriteMixin):
     fmt = "pcjson"
     maxDiff = None
@@ -2926,7 +2939,7 @@ class TestPCJSON(unittest.TestCase, WriteMixin):
       "charge": 0
     }
   ]
-}""")
+}""", normalize=normalize_pcjson_floats)
 
 # pcm -- PCModel Format
 class TestPCM(unittest.TestCase, WriteMixin):