[WX-1629] Add metadata value type tests #7433

rsaperst · 2024-05-14T15:50:54Z

Follow-up to WX-1410 to add additional testing of metadata value types to ensure that values are inserted into the database with correct types.

jgainerdewar · 2024-05-22T21:07:54Z

centaur/src/main/resources/standardTestCases/draft3_infer_version.test

@@ -13,5 +13,5 @@ metadata {
  status: Succeeded
  "outputs.draft3_infer_version.j": 660
  "actualWorkflowLanguage": WDL
-  "actualWorkflowLanguageVersion": 1.0
+  "actualWorkflowLanguageVersion": "1.0"


Nice one 😂

...r/src/main/resources/standardTestCases/metadata_type_validation/metadata_type_validation.wdl

...rdTestCases/wdl_biscayne/biscayne_new_runtime_attributes/biscayne_new_runtime_attributes.wdl

...rc/main/resources/standardTestCases/wdl_draft3/draft3_infer_version/draft3_infer_version.wdl

jgainerdewar · 2024-05-22T21:19:12Z

centaur/src/main/scala/centaur/test/metadata/WorkflowFlatMetadata.scala

      case JsNull => (expected != JsNull).option(s"expected: $cacheSubstitutions but got: $actual")
      case _ => Option(s"expected: $cacheSubstitutions but got: $actual")
    }

    matchError.map(s"Metadata mismatch for $key - " + _)
  }
+


You've done an admirable job creating your own string-to-array parser here, but I wonder if there's anything available in spray that can do this for us. Nice to outsource JSON serialization and deserialization wherever we can.

Yup, spray has a parseJson method which was able to do this for us!

jgainerdewar · 2024-05-22T21:20:51Z

centaur/src/main/scala/centaur/test/metadata/WorkflowFlatMetadata.scala

-      case Result.Failure(_) => invalidNel(s"Metadata block can not be converted to a Map: $config")
+  def fromConfig(config: Config): ErrorOr[WorkflowFlatMetadata] = {
+    val values: scala.collection.mutable.Map[String, JsValue] = scala.collection.mutable.Map[String, JsValue]()
+    config.entrySet().stream().forEach { value =>


As above - any chance spray can help us with this?

I don't think spray can help us with this one, by this point we aren't dealing with JSON anymore. I looked around for a function that could do this for us and it doesn't seem like any such function exists.

jgainerdewar · 2024-05-22T21:22:05Z

services/src/main/scala/cromwell/services/metadata/impl/WriteMetadataActor.scala

@@ -29,16 +29,7 @@ class WriteMetadataActor(override val batchSize: Int,
  private val statsRecorder = MetadataStatisticsRecorder(metadataStatisticsRecorderSettings)

  override def process(e: NonEmptyVector[MetadataWriteAction]) = instrumentedProcess {


Is this just rearrangement or are there logic changes here?

This is just rearrangement to make the process method more easily unit testable.

...rc/main/resources/standardTestCases/wdl_draft3/draft3_infer_version/draft3_infer_version.wdl

…tal backend

aednichols · 2024-05-28T19:32:43Z

centaur/src/main/resources/standardTestCases/biscayne_read_functions_windows_line_endings.test

@@ -14,9 +14,9 @@ metadata {
  "outputs.biscayne_read_functions_windows_line_endings.map.y": 600
  "outputs.biscayne_read_functions_windows_line_endings.map.z": 700
  "outputs.biscayne_read_functions_windows_line_endings.map.x": 500
-  "outputs.biscayne_read_functions_windows_line_endings.tsv.0": "[\"line1\",\"line one\"]"
-  "outputs.biscayne_read_functions_windows_line_endings.tsv.1": "[\"line2\",\"line two\"]"
-  "outputs.biscayne_read_functions_windows_line_endings.tsv.2": "[\"line3\",\"line three\"]"


This is super nice, I've definitely been annoyed in the past about having to craft just the right escape sequence for a test.

aednichols · 2024-05-28T19:34:59Z

centaur/src/main/scala/centaur/test/Test.scala

      } else {
        IO.unit
      }
    }

+    private def checkIfActuallySame(firstSet: Set[(String, JsValue)], secondSet: Set[(String, JsValue)]): Boolean = {


Naming: this function alludes to a "kinda same"/"actually same" duality... not sure that's what's happening

Essentially the actual outputs are in a "cleaned" state where extraneous quotation marks have been removed, but the expected outputs might have extra quotation marks if the output is a String (e.g., the actual output will be "hello" but the expected output will read ""hello""). This function just cleans the outputs to make sure that we're not reporting a false difference caused by quotes that should have been removed.

Could you add a comment to that affect?

aednichols · 2024-05-28T19:37:41Z

centaur/src/main/scala/centaur/test/metadata/WorkflowFlatMetadata.scala

-      HOCON). However values coming from the metadata endpoint are JsValue. At the moment we're only using
-      JsString, JsNumber and JsBoolean for comparison so this is a hacky way of handling that situation. It's
-      entirely likely that it won't survive long term.
-     */


@geoffjentry we determined that 8 years is the going definition of "long term".

aednichols · 2024-05-28T19:40:37Z

src/ci/bin/testCentaurHoricromtalPapiV2beta.sh

+    # Due to Centaur changes in WX-1629, lots_of_inputs_papiv2 times out due to the large number of inputs. Instead,
+    # we run lots_of_inputs, which tests 400 inputs instead of the 10,000 in lots_of_inputs_papiv2


Aside from this test – did the slower Centaur evaluation inflate the overall suite time? I can't immediately intuit whether it would matter.

Comparing the length of time of the tests on this PR to the nightly integration tests, the timings are about the same (some suites are a little faster, some are a little slower, all are pretty close though). I think that one test was problematic because it creates an array of 10,000 Files, which causes a recursion stack of 10,000 calls when we process the array. No other tests create such large arrays, and I think that call stack might have caused memory issues and slowness (I didn't test this hypothesis, but it would make sense since that seems to be the main difference between this test and others).

👍 sounds fine to me, since I only care about performance to the extent that I have to wait for tests.

jgainerdewar

Nice job wrangling this!

jgainerdewar · 2024-05-28T20:24:50Z

centaur/src/main/scala/centaur/test/Test.scala

      } else {
        IO.unit
      }
    }

+    private def checkIfActuallySame(firstSet: Set[(String, JsValue)], secondSet: Set[(String, JsValue)]): Boolean = {


Could you add a comment to that affect?

centaur/src/main/scala/centaur/test/metadata/WorkflowFlatMetadata.scala

jgainerdewar · 2024-05-28T20:45:23Z

src/ci/resources/tes_application.conf

+
+services {
+    MetadataService {
+        metadata-keys-to-sanitize-utf8mb4 = ["submittedFiles:workflow", "commandLine", "outputs:metadata_type_validation.validate_boolean.boolean_output"]


What was special about outputs:metadata_type_validation.validate_boolean.boolean_output that caused it to be added here? Might want a comment explaining.

There's nothing special about that key, I added it to prove through integration testing that sanitizing metadata keys won't convert non-Strings to Strings (essentially I just picked that one randomly).

...rc/main/resources/standardTestCases/wdl_draft3/draft3_infer_version/draft3_infer_version.wdl

jgainerdewar · 2024-05-29T15:27:09Z

.github/workflows/integration_tests.yml

+#      timeout-minutes: 30
+#      with:
+#        limit-access-to-actor: true
+#        detached: true
    #This script bascially just looks up another script to run, assuming that the other script's filename is:


I assume this is left in here commented-out intentionally and I support it 👍

Yes, that is correct!

rsaperst and others added 25 commits May 14, 2024 11:48

[WX-1629] Add metadata value type tests

e60257d

Updated print

9b57ed5

Changed print to log

a3266a5

Add additional centaur test and unit test

ec872ad

Add extra key to sanitize

6b80c83

Test that true != "true" in Centaur

724c363

Added additional unit tests.

bd434e2

Modify Centaur so that "true" != true

b177495

scalafmt

f0f07c5

Make new test suite to run only metadata tests (temporary)

5c5c627

Modify testCentaurLocal.sh to only run metadata tests (temporary)

a562b86

Slight modification to test suite

8e6899a

Add additional printing

57075c3

Added more logging

1be792d

More logging

0ade3ea

Modified Centaur to make comparisons taking data type into account

1539221

Merge branch 'develop' into wx1629

38b0a42

scalafmt

81f5f80

Reverted yml change

77fe961

Reverted extraneous change

97630aa

Fixed most failing tests

6f340e3

Parse lists from .test files

697d926

Fixed remaining failing tests

b783831

Merge branch 'develop' into wx1629

645e4c7

Remove extraneous log

76a9f1f

rsaperst marked this pull request as ready for review May 22, 2024 20:18

rsaperst requested a review from a team as a code owner May 22, 2024 20:18

Fixed last failing test

6542a5d

jgainerdewar reviewed May 22, 2024

View reviewed changes

aednichols reviewed May 22, 2024

View reviewed changes

...rc/main/resources/standardTestCases/wdl_draft3/draft3_infer_version/draft3_infer_version.wdl Outdated Show resolved Hide resolved

rsaperst added 12 commits May 23, 2024 14:13

Review comments

f45d300

Revert whitespace change

08d7d8d

Test to see if this resolves timeout errors

6c32f1a

Reverted test

8675777

Added SSH step to integration tests

1f3adb1

Don't run lots_of_inputs on Papi

9f10bd1

Fix failing test

9b72152

Modify Papi config and re-enable lots_of_inputs test

f0329b9

Test lots_of_inputs instead of lots_of_inputs_papiv2

1e589e8

Switch from if statements to pattern matching

06e4850

Switch to lots_of_inputs instead of lots_of_inputs_papiv2 on Horicrom…

dd5872a

…tal backend

Cleanup

32810b0

aednichols approved these changes May 28, 2024

View reviewed changes

jgainerdewar reviewed May 28, 2024

View reviewed changes

Review fixes

3ed63d2

jgainerdewar approved these changes May 29, 2024

View reviewed changes

rsaperst merged commit 408951f into develop May 29, 2024
37 checks passed

rsaperst deleted the wx1629 branch May 29, 2024 16:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WX-1629] Add metadata value type tests #7433

[WX-1629] Add metadata value type tests #7433

rsaperst commented May 14, 2024 •

edited

Loading

jgainerdewar May 22, 2024

jgainerdewar May 22, 2024

rsaperst May 23, 2024

jgainerdewar May 22, 2024

rsaperst May 23, 2024

jgainerdewar May 22, 2024

rsaperst May 23, 2024

aednichols May 28, 2024

aednichols May 28, 2024

rsaperst May 28, 2024

jgainerdewar May 28, 2024

aednichols May 28, 2024

aednichols May 28, 2024

rsaperst May 28, 2024

aednichols May 28, 2024

jgainerdewar left a comment

jgainerdewar May 28, 2024

jgainerdewar May 28, 2024

rsaperst May 29, 2024

jgainerdewar May 29, 2024

rsaperst May 29, 2024

		@@ -29,16 +29,7 @@ class WriteMetadataActor(override val batchSize: Int,
		private val statsRecorder = MetadataStatisticsRecorder(metadataStatisticsRecorderSettings)

		override def process(e: NonEmptyVector[MetadataWriteAction]) = instrumentedProcess {

		# Due to Centaur changes in WX-1629, lots_of_inputs_papiv2 times out due to the large number of inputs. Instead,
		# we run lots_of_inputs, which tests 400 inputs instead of the 10,000 in lots_of_inputs_papiv2

[WX-1629] Add metadata value type tests #7433

[WX-1629] Add metadata value type tests #7433

Conversation

rsaperst commented May 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jgainerdewar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rsaperst commented May 14, 2024 •

edited

Loading