Move test code in respective projects #5330

RustedBones · 2024-04-09T16:13:23Z

Splitting scio-test in sub modules

scio-test-core: depending on scio-core + some avro deps for pretty printing
scio-test-google-cloud-platform: depending on scio-test-core + GCP deps
scio-test-parquet: for future helpers

In order to use scio-test-core in scio-core/test scope, I created a symlink to the com.spotify.scio.testing package.

As most scio module test scope require test data, set the dependency on scio-core % "test->test" to get access to test helpers from scio-test-core

format code

codecov · 2024-04-11T13:00:28Z

Codecov Report

Attention: Patch coverage is 75.00000% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 61.37%. Comparing base (6ebf9c4) to head (3c8f12c).
Report is 23 commits behind head on main.

❗ Current head 3c8f12c differs from pull request most recent head 1329726. Consider uploading reports for the commit 1329726 to get more accurate results

Files	Patch %	Lines
...c/main/scala/com/spotify/scio/testing/Pretty.scala	75.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5330      +/-   ##
==========================================
- Coverage   62.69%   61.37%   -1.32%     
==========================================
  Files         301      302       +1     
  Lines       10848    10818      -30     
  Branches      773      786      +13     
==========================================
- Hits         6801     6640     -161     
- Misses       4047     4178     +131

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

RustedBones · 2024-04-11T14:43:31Z

scio-avro/src/test/avro/schema.avsc

-}, {
-    "type": "record",
-    "name": "TestRecord",
-    "namespace": "com.spotify.scio.avro",
-    "doc": "Record for testing",
-    "fields": [
-        {"name": "int_field", "type": ["null", "int"], "default": null},
-        {"name": "long_field", "type": ["null", "long"], "default": null},
-        {"name": "float_field", "type": ["null", "float"], "default": null},
-        {"name": "double_field", "type": ["null", "double"], "default": null},
-        {"name": "boolean_field", "type": ["null", "boolean"], "default": null},
-        {"name": "string_field", "type": ["null", "string"], "default": null},
-        {"name": "array_field", "type": {"type": "array", "items": "string"}, "default": null}
-    ]


This one stayed in scio-core/test to test kryo for avro

RustedBones · 2024-04-11T14:49:10Z

scio-core/src/test/scala/com/spotify/scio/ArgsTest.scala

-    def getInput: String
-    def setInput(input: String): Unit
+    def getTestInput: String
+    def setTestInput(input: String): Unit
    @Required
-    def getOutput: String
-    def setOutput(output: String): Unit


Since PipelineOptions are dynamically loaded, Some tests were failing because the --input and --output parameters were transformed into options instead of staying in the args. Renamed the values to avoid conflicts wit other tests

RustedBones · 2024-04-11T14:49:28Z

scio-core/src/test/scala/com/spotify/scio/avro/AvroUtils.scala

@@ -38,7 +39,7 @@ object AvroUtils {
    .array()
    .items()
    .stringType()
-    .noDefault()
+    .arrayDefault(Collections.emptyList())


to match with the specific record

RustedBones · 2024-04-11T14:50:57Z

scio-core/src/test/scala/com/spotify/scio/coders/KryoAtomicCoderTest.scala

+// additional kryo registrar for avro schema null value
+// deserializes null to the singleton instance for schema equality
+@KryoRegistrar
+class TestKryoRegistrar extends IKryoRegistrar {
+
+  override def apply(k: Kryo): Unit =
+    k.forClass[NullNode](new SingletonSerializer(NullNode.getInstance()))
+}


Apparently, this test suite was not run before.
This registrar is needed for the set so schema equality passes. Without this, kryo creates a new NullNode instance failing equality checks

RustedBones · 2024-04-11T14:51:08Z

scio-core/src/test/scala/com/spotify/scio/coders/KryoAtomicCoderTest.scala

@@ -65,8 +80,7 @@ class KryoAtomicCoderTest extends PipelineSpec {
    Pair("record", 10) kryoCoderShould roundtrip()
  }

-  // Enable once https://github.com/scala/scala/pull/10425 is release


fix is now live

clairemcginty

this is amazing! thanks for your hard work doing this refactor

clairemcginty · 2024-04-11T17:29:48Z

build.sbt

    libraryDependencies ++= Seq(
-      "com.google.api.grpc" % "proto-google-cloud-bigtable-v2" % googleCloudProtoBigTableVersion,
-      "com.google.http-client" % "google-http-client" % googleHttpClientVersion,
+      "com.google.http-client" % "google-http-client" % googleHttpClientVersion, // TODO should we have this here ?


Possible to move to google-cloud-platform test module?

This is required by the Pretty module. Instead of using toString in the scio SCollection matchers, we rely on this with custom formatting for

avro

gson

Ideally we should use smth like the cat's Show typeclass so any etension can give its formatting. Kept as is to avoid breaking the test API.

scio-avro/src/test/scala/com/spotify/scio/avro/AvroJobTestTest.scala

clairemcginty · 2024-04-11T17:32:22Z

scio-avro/src/test/scala/com/spotify/scio/avro/AvroTapTest.scala

+  val schema: Schema = AvroUtils.schema
+  implicit def coder: Coder[GenericRecord] = avroGenericRecordCoder(schema)
+
+  "AvroTap" should "support saveAsAvroFile with SpecificRecord" in withTempDir { dir =>


Wdyt about moving the withTempDir functionality into scio-test-core? It seems to have a lot of usage both within Scio and within many pipeline projects. (doesn't have to be in this PR!)

Will expose this in a separate PR

kellen · 2024-04-12T14:31:07Z

Should we retarget this for 0.15?

RustedBones · 2024-04-12T14:34:12Z

I think @clairemcginty wanted this to ship some test helpers sooner.
Here the mima sais is breaking because classes moved from one artifact to another. There should in practice be no breaking changes as scio-test depends on scio-test-core and scio-test-google-cloud-service containing the same classes as before.

RustedBones · 2024-04-12T14:36:00Z

I can actually try to run mima on scio-test-core with scio-test artifact localy to make sure if I didn't overlooked something

kellen · 2024-04-12T15:10:34Z

Was thinking more for the ease of merging 0.15 later, but if there is an urgent need we can get it out sooner and just deal with the merge issues.

RustedBones · 2024-04-15T13:38:07Z

This is the result of the binary compatibility check

[error] scio-test-core: Failed binary compatibility check against com.spotify:scio-test_2.13:0.14.3! Found 1 potential problems
[error]  * interface com.spotify.scio.testing.BigtableMatchers does not have a correspondent in current version
[error]    filter with: ProblemFilters.exclude[MissingClassProblem]("com.spotify.scio.testing.BigtableMatchers")

which is expected because moved to scio-test-google-cloud-platform

RustedBones force-pushed the scio-test-split branch 2 times, most recently from acc8175 to a7e31e4 Compare April 10, 2024 16:22

RustedBones added 10 commits April 11, 2024 11:43

Move test code in respective projects

a468c93

Update GHA

24c29bd

Format sbt

7006d82

Add symlink

b788984

Fix kryo tests

7764680

format code

Fix doc

4b4a387

Format sbt

0fa2ad8

Fix flacky test

99d87ce

Fix build dependencies

7f13d12

Move additional NullNode kryo serializer to test scope

94e96b8

RustedBones force-pushed the scio-test-split branch from 572a8e2 to 94e96b8 Compare April 11, 2024 09:48

RustedBones added 2 commits April 11, 2024 12:26

Fix dependencies declaration

304c745

Disable mima for scio-test modules

3c8f12c

RustedBones force-pushed the scio-test-split branch from 4bac200 to 3c8f12c Compare April 11, 2024 12:40

RustedBones added 2 commits April 11, 2024 15:17

Finalize avro split

e2a4962

Restore Pretty test printer

38e844f

RustedBones commented Apr 11, 2024

View reviewed changes

clairemcginty approved these changes Apr 11, 2024

View reviewed changes

RustedBones marked this pull request as ready for review April 15, 2024 13:38

RustedBones added 3 commits April 17, 2024 11:02

Update copyright year

b405a94

Merge remote-tracking branch 'origin/main' into scio-test-split

15ad249

Update fansi

1329726

RustedBones merged commit c15591d into main Apr 17, 2024
11 checks passed

RustedBones deleted the scio-test-split branch April 17, 2024 09:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move test code in respective projects #5330

Move test code in respective projects #5330

RustedBones commented Apr 9, 2024 •

edited

Loading

codecov bot commented Apr 11, 2024 •

edited

Loading

RustedBones Apr 11, 2024

RustedBones Apr 11, 2024

RustedBones Apr 11, 2024

RustedBones Apr 11, 2024 •

edited

Loading

RustedBones Apr 11, 2024

clairemcginty left a comment

clairemcginty Apr 11, 2024

RustedBones Apr 12, 2024 •

edited

Loading

clairemcginty Apr 11, 2024

RustedBones Apr 17, 2024

kellen commented Apr 12, 2024

RustedBones commented Apr 12, 2024

RustedBones commented Apr 12, 2024

kellen commented Apr 12, 2024

RustedBones commented Apr 15, 2024

Move test code in respective projects #5330

Move test code in respective projects #5330

Conversation

RustedBones commented Apr 9, 2024 • edited Loading

codecov bot commented Apr 11, 2024 • edited Loading

Codecov Report

RustedBones Apr 11, 2024

Choose a reason for hiding this comment

RustedBones Apr 11, 2024

Choose a reason for hiding this comment

RustedBones Apr 11, 2024

Choose a reason for hiding this comment

RustedBones Apr 11, 2024 • edited Loading

Choose a reason for hiding this comment

RustedBones Apr 11, 2024

Choose a reason for hiding this comment

clairemcginty left a comment

Choose a reason for hiding this comment

clairemcginty Apr 11, 2024

Choose a reason for hiding this comment

RustedBones Apr 12, 2024 • edited Loading

Choose a reason for hiding this comment

clairemcginty Apr 11, 2024

Choose a reason for hiding this comment

RustedBones Apr 17, 2024

Choose a reason for hiding this comment

kellen commented Apr 12, 2024

RustedBones commented Apr 12, 2024

RustedBones commented Apr 12, 2024

kellen commented Apr 12, 2024

RustedBones commented Apr 15, 2024

RustedBones commented Apr 9, 2024 •

edited

Loading

codecov bot commented Apr 11, 2024 •

edited

Loading

RustedBones Apr 11, 2024 •

edited

Loading

RustedBones Apr 12, 2024 •

edited

Loading