Feat - submit tests (#442)

Update test suite with basic submission test
---------

Co-authored-by: nf-osi[bot] <nf-osi@sagebionetworks.org>
Co-authored-by: Robert Allaway <allaway@users.noreply.github.com>

.github/workflows/main-ci.yml

@@ -98,7 +98,17 @@ jobs:
         continue-on-error: true
         run: ./run_config.sh
 
-      # TODO Step for test submission of data
+      - name: Test submit
+        working-directory: tests/submit
+        continue-on-error: true
+        # Only test submit needs SYNAPSE_AUTH_TOKEN
+        env:
+          SYNAPSE_AUTH_TOKEN: ${{ secrets.SYNAPSE_AUTH_TOKEN }}
+        # Need to set up csvtool for use in test environment
+        run: |
+          sudo apt install csvtool
+          printf "[authentication]\nSYNAPSE_AUTH_TOKEN=$SYNAPSE_AUTH_TOKEN" > .synapseConfig
+          ./test_submit.sh
       
       - name: Create test suite report
         working-directory: tests

.gitignore

@@ -1,13 +1,18 @@
-# Dist and test artifacts 
+# Secrets
 .synapseConfig
 creds.json
+
+# Dist and test artifacts 
 tests/generate/creds.json
 tests/generate/NF.jsonld
 tests/generate/config.json
+tests/validate/great-expectations
+tests/submit/work
 tests/test-suite-report.md
 tests/**/logs
 publish/**
 
+**/schematic
 *.schema.json
 *.manifest.csv
 *.test.xlsx

tests/README.md

@@ -1,36 +1,44 @@
-## Test documentation (WIP)
+## Test documentation
 
 This describes the testing framework for the data model. 
 The test suite is organized by functionality: 
 - Test GENERATION of templates
 - Test VALIDATION of manifests against templates
 - Test SUBMISSION of manifests
 
+Tests are *mostly* self-contained (don't depend on output from other tests) and should be run in their respective directory;
+the exceptions and details are in the following sections.
+
+These correspond to [`schematic manifest get`](https://sage-schematic.readthedocs.io/en/develop/cli_reference.html#schematic-manifest-get), [`schematic model validate`](https://sage-schematic.readthedocs.io/en/develop/cli_reference.html#schematic-model-validate), and [`schematic model submit`](https://sage-schematic.readthedocs.io/en/develop/cli_reference.html#schematic-model-submit), respectively.
 Tests are pretty self-contained (don't depend on output from other tests) and should be run in their respective directory.
 Details are in the following dedicated sections.
 
 ### Test suite
 
-#### Test GENERATION of templates (:white_check_mark: SEMI-IMPLEMENTED)
+#### Test GENERATION of manifest templates
+
+> [!NOTE]
+> This can be run independently, i.e. it *does not* on any other tests here (though it does depend on the JSON-LD build).
 
 This means means checking expectations that:
-1. Templates can be generated at all with the version of `schematic` used. 
-2. Templates look as expected when generated:
-    - As a basic blank template not referring to any specific files
-    - As a template instance with unannotated data filenames filled in
-    - As a template for data files with *existing* annotations pulled in,
-        - where existing annotations are compatible
-        - where existing annotations are incompatible (schema changes have happened in-between)
+- [x] Templates can be generated at all with the version of `schematic` used, for the current JSON-LD data model. 
+- [x] Templates look as expected when generated for a new dataset of a particular data type (no existing annotations).
+- [ ] Templates look as expected when generated for a dataset of a particular data type when pulling in existing annotations. 
 
 At the most basic template generation mode, failures with a given version of `schematic` could mean:
-- Version of `schematic` is too old and doesn't support whatever new validation rule/feature used in the model. This gives us information on the minimum compatible version.
+- Version of `schematic` is too old and doesn't support whatever new validation rule syntax/feature used in the model. This gives us information on the minimum compatible version.
 - Version of `schematic` is too new and contains *breaking changes* that won't work with the current data model.  
 - There's a problem with the data model such as a missing entity. For example, if a template uses a property (key) but that property is undefined in the data model so the schematic will complain at template generation time. 
 
 At a more complicated template generation mode (with existing data involved), failures could mean:
 - The template has been updated in a way that is not compatible with past versions/hard to handle. That means that a contributor trying to update their data with this new template will run into an error as well.
 - Edge cases that's hard to handle within data model itself. Example: data has been annotated outside of DCA and doesn't conform to a template so schematic is confused when generating a template. 
 
+###### Note on "advanced" manifest template generation
+
+Manifests templates can be generated with values filled in from previous submissions, instead of a blank template with just entity ids/names. 
+However, this mode is not currently tested. It is also more DCC-variable, because existing values can be sourced from annotations, tables, or files as several different sources of truth.
+
 ##### Test fixtures
 
 To test **basic** generation we need:
@@ -46,15 +54,23 @@ To test **advanced** generation as described above we additionally need:
 
 - (in `tests/generate/`) `./basic_templates.sh` (generate basic blank GoogleSheets templates)
 
-#### Test VALIDATION of manifests against their templates (:heavy_check_mark: IMPLEMENTED)
+#### Test VALIDATION of manifests against their templates
+
+> [!NOTE]
+> This can be run independently, i.e. it *does not* depend on test GENERATION.
+
+> [!TIP]
+> The test script should match the parameters used by your [DCA config](https://github.com/Sage-Bionetworks/data_curator_config/).
+> The script included here may not match your DCC's configuration, modify as needed if reusing.
+> For example, some DCCs use Great Expectations rules, some don't, and this is controlled by the `-rr` flag. 
 
 Question: Why do testing in addition to [the tests](https://github.com/Sage-Bionetworks/schematic/tree/develop/tests/data/mock_manifests) run by `schematic`?  
 Answer: We sometimes have issues that are not covered by those tests or that we want to check specifically in the context of our own templates. 
 Also, if there are unintuitive or unclear validation behaviors, it's a good idea to just create a mock manifest that captures that so you have it documented and validated how something works.
 
 This means checking expectations that:
-1. Manifest data that should pass are indeed passed by `schematic`. Example issue: https://github.com/Sage-Bionetworks/schematic/issues/732. Issues in this case lead to a poor experience for data contributors, who wouldn't appreciate spurious validation errors. 
-2. Manifest data that should fail are indeed caught by `schematic`. Issues in this case lead to letting bad data pass and are arguably worse.  
+- [x] Manifest data that should pass are indeed passed by `schematic`. Example issue: https://github.com/Sage-Bionetworks/schematic/issues/732. Issues in this case lead to a poor experience for data contributors, who wouldn't appreciate spurious validation errors. 
+- [x] Manifest data that should fail are indeed caught by `schematic`. Issues in this case lead to letting bad data pass and are arguably worse.  
 
 ##### Test fixtures 
 
@@ -72,32 +88,34 @@ To test validation we need:
 
 Not to be confused with test generation section above, generative testing means generating manifest data for testing, similar to the idea of "synthetic data". Currently the "clean and dirty" manifests are created manually -- which can be a lot of work to have to sit down and think of all the weird things and how they should be handled by schematic validation. 
 
-#### Test SUBMISSION of manifests (:warning: TODO)
+#### Test SUBMISSION of manifests
 
-> Note: This is more complicated than the other two test suites combined.
+> [!NOTE]
+> This *does* depend on the VALIDATION test suite being run, because only passing manifests from that will be submitted.  
+Currently, we have partial implementation as seen in the checked features below.
 
 This means checking that:
-1. Valid manifests can be submitted at all. There have been cases where valid manifests are validated OK but unable to be submitted. 
-2. Manifest data are transferred as expected to Synapse (e.g. no weird conversions of types or truncation of data). This requires querying the data that has been transferred to Synapse for comparison. Example issues: 
-- Integers are uploaded as doubles https://github.com/Sage-Bionetworks/schematic/issues/664
-- Blank values in the manifest become "NA" -- https://github.com/Sage-Bionetworks/schematic/issues/733
-- "NA" string value become `null` even though we may want to preserve "NA" -- https://github.com/Sage-Bionetworks/schematic/issues/821 + [internal thread](https://sagebionetworks.slack.com/archives/C01ANC02U59/p1681769606510569?thread_ts=1681769370.017039&cid=C01ANC02U59)
-
-The test script will need to do several things:
-- Submit the `.csv` manifest(s) via schematic.
-- After submission (if submission is successful), pull annotations from the entities and does expectation checks on each key-value.
+- [x] Valid manifests can be submitted at all. There have been cases where valid manifests are validated OK but unable to be submitted. Sometimes it could be a cloud service issue when using the public schematic API; testing submission with the pure client (without the cloud layer) helps better distinguish where the error is coming from. Example issues:
+      - https://github.com/Sage-Bionetworks/data_curator/issues/393
+      - https://sagebionetworks.jira.com/browse/FDS-1968
+- [ ] Manifest data are transferred as expected to Synapse (e.g. no weird conversions of types or truncation of data).
+      This is the most complicated functionality across the test suite and requires querying the data that have been transferred to Synapse for comparison. Example issues: 
+  - Integers are uploaded as doubles https://github.com/Sage-Bionetworks/schematic/issues/664
+  - Blank values in the manifest become "NA" -- https://github.com/Sage-Bionetworks/schematic/issues/733
+  - "NA" string value become `null` even though we may want to preserve "NA" -- https://github.com/Sage-Bionetworks/schematic/issues/821 + [internal thread](https://sagebionetworks.slack.com/archives/C01ANC02U59/p1681769606510569?thread_ts=1681769370.017039&cid=C01ANC02U59)
+
+The test script functionality:
+- Given valid `.csv` manifest(s), create entities in the manifest, and then update the manifest with the `entityId`s to test submit via schematic, then cleans up (deletes) mock entities.
+- (TODO) After submission (if submission is successful), pull annotations from the entities and does expectation checks on each key-value.
 
 ##### Test fixtures
 
 To test submission we need:
-- Synapse entities to be mock-annotated and checked, e.g. in [NF-test/annotations](https://www.synapse.org/#!Synapse:syn32530621)
-- Manifest to submit for entities
-
+- valid manifest(s)  -- these are reused from VALIDATION fixtures.
+- a `config.json` to specify Synapse test project and paths to validation fixtures
 
-### TODOs - General
+### General Testing TODO Ideas and Other Tips
 
 - Testing can transition to using the schematic API service once it's ready (especially for template generation).
-- Improve test log output/parse and format results to have clearer summary of outcomes without having to read raw test logs  
-- Reuse more code between tests
-- See if test scripts can be generalized/reused for other projects
+- How test scripts can be generalized/reused for other projects
 - Main script to run through test suite in sequence