Failing tests in JmolParserTest

Jalview Developers
1

These failed for me unexpectedly:

JmolParserTest.testFileParser()

JmolParserTest.testAlignmentLoader()

I eventually tracked it down (it took a while):

  • test setup sets ADD_SS_ANN and STRUCT_FROM_PDB to TRUE, but didn’t explicitly set ADD_TEMPFACT_ANN to FALSE
  • I had this set to TRUE in my properties file, so two annotations per sequence were created
  • the assertion on the first annotation on the sequence reported ‘No secondary structure assigned’, but it actually failed on ‘annotation.hasIcons’
    • the secondary structure annotation was created fine, but was the second annotation, and Temp Factor fails ‘hasIcons’
      Fix: set ADD_TEMPFACT_ANN to FALSE in setup

Moral 1: use separate assertions for different failure conditions

Moral 2: always use a test properties file for testing under controlled conditions e.g.

Cache.loadProperties(“test/jalview/io/testProps.jvprops”);

It took time to debug because the preferences passing is so complicated:

  • the JmolParser constructor call in testFileParser() has addAlignmentAnnotations = false…?
    • hang on, that parameter isn’t used; it should be either removed or used (as in PDBFile constructor)
  • in fact, the preferences for ‘annotation from structure’ etc are set but never read in testFileParser
    • they are read from Cache in FormatAdapter.init(), then transferred to a singleton with StructureImportSettings.addSettings(), then read in StructureFile.xferSettings() (from JmolParser) or StructureFile.addSettings() (from PBDFile constructors), then transferred in the PDBChain constructor, then read in makeResidueList()
    • but testFileParser bypasses the first two steps (goes direct to the JmolParser constructor) so the preferences are never read
    • sure enough, if I turn off secondary structure in my preferences, and run testFileParser() on its own, it fails
      • it passes when JmolParserTest is run because other tests initialise the StructureImportSettings singleton (I think)

This all looks unnecessarily complicated, and makes debugging tedious. Is there any reason why we can’t simply read the properties from Cache when needed?
If there is an objection that it needs a test of two flags combined, and that is application logic that shouldn’t be in Cache, then let StructureImportSettings do that, but not copy / cache / store / propagate the actual values.

mungo

The University of Dundee is a registered Scottish Charity, No: SC015096

···

Mungo Carstairs
Jalview Computational Scientist
The Barton Group
Division of Computational Biology
School of Life Sciences
University of Dundee, Dundee, Scotland, UK.
www.jalview.org
www.compbio.dundee.ac.uk