dcc88beac1cdad270235e85e456adafd5e201ceb
braney
  Wed Sep 16 12:39:55 2026 -0700
hgTablesTest: catch an abort per test rather than losing the run, refs #38356

The guards added for #38356 covered the three ways a track page could come
back unusable.  Fifteen other errAbort calls in this program were left, in the
output tests and in the per-database setup, and any one of them still ended the
run with no summary written.

lib/qa.c has done the right thing all along: qaPageGet and qaPageFromForm
bracket the fetch and the validation in an errCatch and hand back a qaStatus
carrying the message.  That is why an unparseable page was already survivable.
The test never applied the same pattern to its own code, though it has included
errCatch.h all along.

So bracket each unit of work: one table, one track, one group, one database,
and each of the three uniProt tests that run at the end.  Each body keeps its
code and moves behind a wrapper of the same name, so the diff is the wrapper
and nothing else.  recordAbort turns a caught abort into a hard error in the
summary with the organism, database, group, track and table that produced it.
That step is needed because quickSubmit records how the page fetch went, and
nothing recorded what a test then made of a page it did get.

The three guards stay as they are.  In those cases quickSubmit has already
recorded the failure, so restoring the errAbort and catching it again would
count one failure twice.  The guards are the already-recorded path and the
errCatch is the not-recorded-anywhere path.

The exit code has to carry the same news.  main returned zero whatever
happened, which was harmless while the first bad page aborted the process, and
is not harmless now that the run carries on: a caller reading only the exit
code would be told that a run failing on every track had succeeded.  Return 1
when any test hit a hard error, or when no test ran at all.  Soft errors do not
set it.  There the page was read and the answer was wrong, which is a report
about hgTables rather than about this run, and it is still in the summary.

The root page fetch stays fatal.  If the front page cannot be read there is
nothing to test.

diff --git src/hg/hgTablesTest/hgTablesTest.c src/hg/hgTablesTest/hgTablesTest.c
index 8019dd500bf..2f3a6bdf151 100644
--- src/hg/hgTablesTest/hgTablesTest.c
+++ src/hg/hgTablesTest/hgTablesTest.c
@@ -138,30 +138,49 @@
 test->info[ntiiTrack] = cloneString(naForNull(track));
 test->info[ntiiTable] = cloneString(naForNull(table));
 slAddHead(&tablesTestList, test);
 return test;
 }
 
 void tablesTestLogOne(struct tablesTest *test, FILE *f)
 /* Log test result to file. */
 {
 int i;
 for (i=0; i<ArraySize(test->info); ++i)
     fprintf(f, "%s ", test->info[i]);
 fprintf(f, "%s\n", test->status->errMessage);
 }
 
+static void recordAbort(char *message, char *type, char *org, char *db,
+	char *group, char *track, char *table)
+/* Turn a caught errAbort into a hard error in the summary.  quickSubmit records
+ * how the page fetch went; nothing records what a test made of the page it got,
+ * so without this an abort inside a test would vanish from the counts. */
+{
+struct qaStatus *qs;
+AllocVar(qs);
+qs->hardError = TRUE;
+qs->errMessage = cloneString(message);
+tablesTestNew(qs, type, org, db, group, track, table);
+verbose(1, "Caught abort testing %s (%s %s %s %s %s): %s",
+	type, naForNull(org), naForNull(db), naForNull(group),
+	naForNull(track), naForNull(table), naForNull(message));
+fprintf(logFile, "Caught abort testing %s (%s %s %s %s %s): %s",
+	type, naForNull(org), naForNull(db), naForNull(group),
+	naForNull(track), naForNull(table), naForNull(message));
+}
+
 struct htmlPage *quickSubmit(struct htmlPage *basePage,
 	char *org, char *db, char *group, char *track, char *table,
 	char *testName, char *button, char *buttonVal)
 /* Submit page and record info.  Return NULL if a problem. */
 {
 struct htmlPage *page = NULL;
 
 // don't get ahead of the botDelay
 sleep1000(5000);
 
 verbose(2, "quickSubmit(%p, %s, %s, %s, %s, %s, %s, %s, %s)\n",
 	basePage, naForNull(org), naForNull(db), naForNull(group), 
 	naForNull(track), naForNull(table), naForNull(testName), 
 	naForNull(button), naForNull(buttonVal));
 if (basePage != NULL)
@@ -701,31 +720,31 @@
 
 htmlPageFree(&outPage);
 }
 	
 boolean isObsolete(char *table)
 /* Some old table types we can't handle.  Just warn that
  * they are there and skip. */
 {
 boolean obsolete = sameString(table, "wabaCbr");
 if (obsolete)
     qaStatusSoftError(tablesTestList->status, 
 	"Skipping obsolete table %s", table);
 return obsolete;
 }
 
-void testOneTable(struct htmlPage *trackPage, char *org, char *db,
+static void testOneTableBody(struct htmlPage *trackPage, char *org, char *db,
 	char *group, char *track, char *table)
 /* Test stuff on one table if we haven't already tested this table. */
 {
 /* Why declared here and not globally? */
 static struct hash *uniqHash = NULL;
 char fullName[256];
 if (uniqHash == NULL)
      uniqHash = newHash(0);
 safef(fullName, sizeof(fullName), "%s.%s", db, table);
 if (!hashLookup(uniqHash, fullName))
     {
     struct htmlPage *tablePage;
     struct htmlForm *mainForm;
 
     hashAdd(uniqHash, fullName, NULL);
@@ -793,31 +812,46 @@
 			testOneField(tablePage, mainForm, org, db, group, track, table, rowCount);
 		    }
 		else
 		    {
 			  verbose(1, "%s.%s tableRows=%d, too large >= 500000, skipping.\n", db, table, tableRows);
 		    fprintf(logFile, "%s.%s tableRows=%d, too large >= 500000, skipping.\n", db, table, tableRows);
 		    }
 		}
 	    }
 	htmlPageFree(&tablePage);
 	}
     carefulCheckHeap();
     }
 }
 
-void testOneTrack(struct htmlPage *groupPage, char *org, char *db,
+void testOneTable(struct htmlPage *trackPage, char *org, char *db,
+	char *group, char *track, char *table)
+/* Test one table, surviving an abort from anything it calls.  Most of the
+ * errAborts in this program are in the output tests below testOneTableBody, and
+ * any one of them used to end the whole run. */
+{
+struct errCatch *errCatch = errCatchNew();
+if (errCatchStart(errCatch))
+    testOneTableBody(trackPage, org, db, group, track, table);
+errCatchEnd(errCatch);
+if (errCatch->gotError)
+    recordAbort(errCatch->message->string, "table", org, db, group, track, table);
+errCatchFree(&errCatch);
+}
+
+static void testOneTrackBody(struct htmlPage *groupPage, char *org, char *db,
 	char *group, char *track, int maxTables)
 /* Test a little something on up to maxTables in one track. */
 {
 struct htmlPage *trackPage = quickSubmit(groupPage, org, db, group, 
 	track, NULL, "selectTrack", hgtaTrack, track);
 struct htmlForm *mainForm;
 struct htmlFormVar *tableVar;
 struct slName *table;
 int tableIx;
 
 /* A track whose page does not come back, or comes back unusable, is skipped
  * rather than fatal.  quickSubmit has already recorded the failure in
  * tablesTestList, so it is counted in the final summary either way.  Aborting
  * here used to end the whole run, which meant the summary that carries the
  * error counts was never written at all, and a single bad page - often just a
@@ -849,31 +883,44 @@
     shuffleList(&tableVar->values);
 
 for (table = tableVar->values, tableIx = 0; 
 	table != NULL && tableIx < maxTables; 
 	table = table->next)
     {
     if (clTable && !sameString(clTable, table->name))
 	continue;
     testOneTable(trackPage, org, db, group, track, table->name);
     ++tableIx;
     }
 /* Clean up. */
 htmlPageFree(&trackPage);
 }
 
-void testOneGroup(struct htmlPage *dbPage, char *org, char *db, char *group, 
+void testOneTrack(struct htmlPage *groupPage, char *org, char *db,
+	char *group, char *track, int maxTables)
+/* Test one track, surviving an abort. */
+{
+struct errCatch *errCatch = errCatchNew();
+if (errCatchStart(errCatch))
+    testOneTrackBody(groupPage, org, db, group, track, maxTables);
+errCatchEnd(errCatch);
+if (errCatch->gotError)
+    recordAbort(errCatch->message->string, "track", org, db, group, track, NULL);
+errCatchFree(&errCatch);
+}
+
+static void testOneGroupBody(struct htmlPage *dbPage, char *org, char *db, char *group, 
 	int maxTracks)
 /* Test a little something on up to maxTracks in one group */
 {
 struct htmlPage *groupPage = quickSubmit(dbPage, org, db, group, NULL, NULL,
 	"selectGroup", hgtaGroup, group);
 struct htmlForm *mainForm;
 struct htmlFormVar *trackVar;
 struct slName *track;
 int trackIx;
 
 /* As in testOneTrack, a group we cannot read is skipped rather than fatal, so
  * that one bad page does not cost the whole run. */
 if (groupPage == NULL)
     {
     verbose(1, "Skipping group %s: no page returned\n", group);
@@ -901,30 +948,43 @@
 
 for (track = trackVar->values, trackIx = 0; 
 	track != NULL && trackIx < maxTracks; 
 	track = track->next)
     {
     if (clTrack && !sameString(track->name, clTrack))
 	continue;
     testOneTrack(groupPage, org, db, group, track->name, clTables);
     ++trackIx;
     }
 
 /* Clean up. */
 htmlPageFree(&groupPage);
 }
 
+void testOneGroup(struct htmlPage *dbPage, char *org, char *db, char *group,
+	int maxTracks)
+/* Test one group, surviving an abort. */
+{
+struct errCatch *errCatch = errCatchNew();
+if (errCatchStart(errCatch))
+    testOneGroupBody(dbPage, org, db, group, maxTracks);
+errCatchEnd(errCatch);
+if (errCatch->gotError)
+    recordAbort(errCatch->message->string, "group", org, db, group, NULL, NULL);
+errCatchFree(&errCatch);
+}
+
 void testGroups(struct htmlPage *dbPage, char *org, char *db, int maxGroups)
 /* Test a little something in all groups for dbPage. */
 {
 struct htmlForm *mainForm;
 struct htmlFormVar *groupVar;
 struct slName *group;
 int groupIx;
 
 if ((mainForm = htmlFormGet(dbPage, "mainForm")) == NULL)
     errAbort("Couldn't get main form on dbPage");
 if ((groupVar = htmlFormVarGet(mainForm, hgtaGroup)) == NULL)
     errAbort("Can't find group var");
 for (group = groupVar->values, groupIx=0; 
 	group != NULL && groupIx < maxGroups; 
 	group = group->next)
@@ -1011,46 +1071,60 @@
 struct dbDb *dbList = hDbDbList(), *db;
 int count = 0;
 
 for (db = dbList; db != NULL && count < maxDbs; db = db->next)
     {
     if (isTestableDb(db) && sameWord(db->organism, org))
 	{
 	slNameAddTail(&list, db->name);
 	++count;
 	}
     }
 dbDbFreeList(&dbList);
 return list;
 }
 
-void testDb(struct htmlPage *orgPage, char *org, char *db)
+static void testDbBody(struct htmlPage *orgPage, char *org, char *db)
 /* Test on one database. */
 {
 struct htmlPage *dbPage;
 char region[256];
 htmlPageSetVar(orgPage, NULL, "db", db);
 getTestRegion(db, region, sizeof(region));
 htmlPageSetVar(orgPage, NULL, "position", region);
 htmlPageSetVar(orgPage, NULL, hgtaRegionType, "range");
 dbPage = quickSubmit(orgPage, org, db, NULL, NULL, NULL, "selectDb", "submit", "go");
 if (dbPage != NULL)
     testGroups(dbPage, org, db, clGroups);
 htmlPageFree(&dbPage);
 }
 
 
+void testDb(struct htmlPage *orgPage, char *org, char *db)
+/* Test one database, surviving an abort.  The setup steps here - the test
+ * region, the group list - abort on their own, and one bad database should not
+ * cost the databases after it. */
+{
+struct errCatch *errCatch = errCatchNew();
+if (errCatchStart(errCatch))
+    testDbBody(orgPage, org, db);
+errCatchEnd(errCatch);
+if (errCatch->gotError)
+    recordAbort(errCatch->message->string, "db", org, db, NULL, NULL, NULL);
+errCatchFree(&errCatch);
+}
+
 void testOrg(struct htmlPage *rootPage, struct htmlForm *rootForm, char *org)
 /* Test on organism.  */
 {
 struct slName *dbList, *db;
 
 /* There is no organism round-trip left to make: the gateway's clade/organism/
  * assembly dropdowns and the "Go" button that submitted them are gone from the
  * page, so submitting for an "organism page" just yields a page with no
  * mainForm.  Name the assembly directly instead, the way the -db= path does --
  * testDb sets db, position and region type on each pass, so rootPage needs no
  * preparation here. */
 dbList = dbsForOrganism(org, clDbs);
 if (dbList == NULL)
     errAbort("No active assembly in dbDb for organism %s", org);
 for (db = dbList; db != NULL; db = db->next)
@@ -1294,60 +1368,87 @@
     struct qaStatistics *stats;
     AllocVar(stats);
     for (test = list; test != NULL; test = test->next)
         {
 	if (sameString(type->name, test->info[subIx]))
 	    {
 	    qaStatisticsAdd(stats, test->status);
 	    }
 	}
     qaStatisticsReport(stats, type->name, f);
     freez(&stats);
     }
 }
 
 
+static int countHardErrors(struct tablesTest *list)
+/* Count the tests that ended in a hard error. */
+{
+int count = 0;
+struct tablesTest *test;
+for (test = list; test != NULL; test = test->next)
+    if (test->status->errMessage != NULL && test->status->hardError)
+        ++count;
+return count;
+}
+
 void reportSummary(struct tablesTest *list, FILE *f)
 /* Report summary of test results. */
 {
 struct qaStatistics *stats;
 struct tablesTest *test;
 int i;
 
 AllocVar(stats);
 for (i=0; i<ntiiTotalCount; ++i)
     statsOnSubsets(list, i, f);
 for (test = list; test != NULL; test = test->next)
     qaStatisticsAdd(stats, test->status);
 fprintf(f, "\ngrand total\n");
 qaStatisticsReport(stats, "Total", f);
 }
 
 
 void reportAll(struct tablesTest *list, FILE *f)
 /* Report all tests. */
 {
 struct tablesTest *test;
 for (test = list; test != NULL; test = test->next)
     {
     if (test->status->errMessage != NULL)
 	tablesTestLogOne(test, f);
     }
 }
 
-void hgTablesTest(char *url, char *logName)
-/* hgTablesTest - Test hgTables web page. */
+static void catchRootTest(void (*test)(struct htmlPage *rootPage), char *name,
+	struct htmlPage *rootPage)
+/* Run one of the whole-program uniProt tests, surviving an abort.  These run
+ * last, so an abort in the first of them used to take the other two and the
+ * summary with it. */
+{
+struct errCatch *errCatch = errCatchNew();
+if (errCatchStart(errCatch))
+    test(rootPage);
+errCatchEnd(errCatch);
+if (errCatch->gotError)
+    recordAbort(errCatch->message->string, name, NULL, "uniProt", NULL, NULL, NULL);
+errCatchFree(&errCatch);
+}
+
+int hgTablesTest(char *url, char *logName)
+/* hgTablesTest - Test hgTables web page.  Returns the exit code: zero only if
+ * the run finished and no test hit a hard error. */
 {
 /* Get default page, and open log. */
 struct htmlPage *rootPage = htmlPageGet(url);
 if (appendLog)
     logFile = mustOpen(logName, "a");
 else
     logFile = mustOpen(logName, "w");
 if (! endsWith(url, "hgTables"))
     warn("Warning: first argument should be a complete URL to hgTables, "
 	 "but doesn't look like one (%s)", url);
 
 fprintf(logFile,"seed=%d\n",seed);
  
 showRunningHostName();
 
@@ -1374,56 +1475,82 @@
 	testOrg(rootPage, mainForm, clOrg);
     else
 	{
 	struct slName *orgList = organismsToTest(clOrgs), *org;
 	if (orgList == NULL)
 	    errAbort("No active organisms in dbDb");
 	for (org = orgList; org != NULL; org = org->next)
 	    {
 	    testOrg(rootPage, mainForm, org->name);
 	    }
 	slNameFreeList(&orgList);
 	}
     }
 
 /* Do some more complex tests on uniProt. */
-testJoining(rootPage);
-testFilter(rootPage);
-testIdentifier(rootPage);
+catchRootTest(testJoining, "joining", rootPage);
+catchRootTest(testFilter, "filter", rootPage);
+catchRootTest(testIdentifier, "identifier", rootPage);
 
 /* Clean up and report. */
 htmlPageFree(&rootPage);
 slReverse(&tablesTestList);
 reportSummary(tablesTestList, stdout);
 reportAll(tablesTestList, logFile);
 fprintf(logFile, "---------------------------------------------\n");
 reportSummary(tablesTestList, logFile);
+
+/* A run that tested nothing, or that could not read a page it asked for, is
+ * not a pass.  Before #38356 the first unreadable page ended the run with
+ * errAbort, so the caller at least saw a nonzero exit.  Now that the run
+ * carries on and counts such a page as a hard error, the exit code has to
+ * carry the same news, or a caller reading only the exit code is told a run
+ * that failed on every track succeeded.  Soft errors are deliberately not
+ * counted here: the page was read and the answer was wrong, which is a report
+ * about hgTables rather than about this run.  They are still in the summary. */
+int testCount = slCount(tablesTestList);
+int hardCount = countHardErrors(tablesTestList);
+if (testCount == 0)
+    {
+    verbose(1, "No tests ran.\n");
+    fprintf(logFile, "No tests ran.\n");
+    return 1;
+    }
+if (hardCount > 0)
+    {
+    verbose(1, "Exiting nonzero: %d of %d tests hit a hard error.\n",
+	hardCount, testCount);
+    fprintf(logFile, "Exiting nonzero: %d of %d tests hit a hard error.\n",
+	hardCount, testCount);
+    return 1;
+    }
+return 0;
 }
 
 int main(int argc, char *argv[])
 /* Process command line. */
 {
 pushCarefulMemHandler(500000000);
 optionInit(&argc, argv, options);
 if (argc != 3)
     usage();
 seed = optionInt("seed",time(NULL));
       verbose(1,"seed=%d\n",seed);
 srand(seed);
 clDb = optionVal("db", clDb);
 clOrg = optionVal("org", clOrg);
 clGroup = optionVal("group", clGroup);
 clTrack = optionVal("track", clTrack);
 clTable = optionVal("table", clTable);
 clDbs = optionInt("dbs", clDbs);
 clOrgs = optionInt("orgs", clOrgs);
 clGroups = optionInt("groups", clGroups);
 clTracks = optionInt("tracks", clTracks);
 clTables = optionInt("tables", clTables);
 appendLog = optionExists("appendLog");
 noShuffle = optionExists("noShuffle");
 if (clOrg != NULL)
    clOrgs = BIGNUM;
-hgTablesTest(argv[1], argv[2]);
+int status = hgTablesTest(argv[1], argv[2]);
 carefulCheckHeap();
-return 0;
+return status;
 }