Chado prepare step needs automated testing

laceysanderson commented 1 year ago

As the title says: Chado prepare step needs automated testing.

There is some basic testing of this in so far as it's run as part of the docker image build and thus if it errors out all tests fail... BUT clearly true automated testing is better. This came up because the prepare stage is often needed before other testing and when run in tests, @risharde encountered the following error:

1) Drupal\Tests\tripal_chado\Functional\TripalImporterGFF3ImporterTest::testGFFImporterSimpleTest
Drupal\tripal_biodb\Exception\TaskException: Failed to complete schema integration task.
Trying to get property 'table_id' of non-object

/var/www/html/drupal/web/modules/t4d8/tripal_chado/src/Task/ChadoPreparer.php:207
/var/www/html/drupal/web/modules/t4d8/tripal_chado/tests/src/Functional/Plugin/TripalImporterGFF3ImporterTest.php:46
/var/www/html/drupal/vendor/phpunit/phpunit/src/Framework/TestResult.php:728

In addition to automated testing for this step, I think a fixture + method added to the ChadoTestBase which quickly adds the needed things to a test schema is needed.

spficklin commented 1 year ago

I've created a branch to work on this: tv4g2-291-prepare_test

@risharde and I are both hitting on this problem and we're going to have major merge conflicts if we try to make fixes in our two branches. So this branch is to fix all of the issues with testing the prepare, including all connected items (ontology loader).

laceysanderson commented 1 year ago

To summarize progress discussed in cofest today, we just realized that it was not clear that me + @risharde had already started on this before @spficklin did. We have been working in branch tv4g1-issue291-testChadoPrepare.

We are now working to merge in @spficklin's work into this branch. Moving forward:

@spficklin will work as he's able on fixes to the prepare step code so that it doesn't fail in automated testing
@laceysanderson will work on fixes to Tripal DBX which are causing issues during the prepare stage. This may include small tweaks outside of Tripal DBX (i.e. basically just adding existing classes/APIs to the whitelist for Tripal DBX)
@laceysanderson and @risharde will continue work on the ChadoTestBrowserBase->prepareTestChado() functionality which tests should call if they need a prepared chado for their own automated testing.

We will all push often to keep all versions in sync, prevent merge conflicts and duplicated effort.

laceysanderson commented 1 year ago

I've just created a PR to try to merge our efforts with a commit that fixes the merge conflicts that popped up. https://github.com/tripal/t4d8/pull/293/

laceysanderson commented 1 year ago

This is completed in PR https://github.com/tripal/t4d8/pull/297 and https://github.com/tripal/t4d8/pull/304 which are both merged into 9.x-4.x

tripal / t4d8

Chado prepare step needs automated testing #291