Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

CoderTest changes for v0.13.x #4806

Merged
merged 7 commits into from
May 18, 2023
Merged

CoderTest changes for v0.13.x #4806

merged 7 commits into from
May 18, 2023

Conversation

shnapz
Copy link
Contributor

@shnapz shnapz commented May 15, 2023 •
edited
Loading

Whatever changes were in master re:Coder,CodetTest are manually integrated in this PR.
upd: more context
this is a merge of changes from #4664:

Added more types of checks to should syntax in coder tests
broke down a coder file and coder test spec into multiple files
removed some redundant checks in CoderAssertions
cleaned up the coder spec

shnapz and others added 4 commits February 10, 2023 16:32
@shnapz @RustedBones
Improve coder tests (#4664)
86eccae 
* Refactor coder tests

* Fixes

* fix scala 2.12 errors

* One more fix

* One more fix

* fix

* one more fix

* one more fix

* one more fix

* added file headers

* added more checks on primitives

* Fixing checks failures

* Fixing checks

* Fix tests

* Removed coderShouldThrowOn

* fix

* Added more checks and fixed a few coders

* Update scio-test/src/test/scala/com/spotify/scio/coders/CoderTest.scala

Co-authored-by: Michel Davit <micheld@spotify.com>

* fixes after review

* Fixing WrappedArray issue

* fix format issue

* Ran scalafixAll on scio-core

* modify syntax with custom options

* fix

---------

Co-authored-by: Michel Davit <micheld@spotify.com>
@shnapz
Merge remote-tracking branch 'origin/main' into v0.13.x
7522304
@shnapz
Merge remote-tracking branch 'origin/main' into v0.13.x
f0da6b0
@shnapz
Merge remote-tracking branch 'origin/main' into v0.13.x
0c5f1a7
@shnapz shnapz requested a review from RustedBones May 15, 2023 18:31
@codecov
Copy link

codecov bot commented May 15, 2023 •
edited
Loading

Codecov Report

Merging #4806 (61a1d40) into main (5a5424d) will increase coverage by 1.15%.
The diff coverage is 90.15%.

❗ Current head 61a1d40 differs from pull request most recent head ad65473. Consider uploading reports for the commit ad65473 to get more accurate results

@@            Coverage Diff             @@
##             main    #4806      +/-   ##
==========================================
+ Coverage   61.25%   62.40%   +1.15%     
==========================================
  Files         286      280       -6     
  Lines       10570    10407     -163     
  Branches      772      760      -12     
==========================================
+ Hits         6475     6495      +20     
+ Misses       4095     3912     -183
Impacted Files Coverage Δ
...otify/scio/coders/LowPriorityCoderDerivation.scala 97.43% <ø> (ø)
...com/spotify/scio/coders/instances/JavaCoders.scala 92.72% <ø> (ø)
...om/spotify/scio/elasticsearch/CoderInstances.scala 100.00% <ø> (ø)
...m/spotify/scio/elasticsearch/ElasticsearchIO.scala 34.48% <ø> (ø)
...la/com/spotify/scio/elasticsearch/IndexAdmin.scala 0.00% <ø> (ø)
...scala/com/spotify/scio/elasticsearch/package.scala 100.00% <ø> (ø)
...in/scala/com/spotify/scio/coders/CustomCoder.scala 84.90% <84.90%> (ø)
...n/scala/com/spotify/scio/coders/WrappedCoder.scala 88.46% <88.46%> (ø)
.../src/main/scala/com/spotify/scio/ScioContext.scala 82.91% <100.00%> (ø)
...src/main/scala/com/spotify/scio/coders/Coder.scala 93.47% <100.00%> (+8.21%) ⬆️
... and 4 more

... and 1 file with indirect coverage changes

@RustedBones
Copy link
Contributor

RustedBones commented May 16, 2023 •
edited
Loading

needs to run scalafix to remove some unused imports

@RustedBones RustedBones added this to the 0.13.0 milestone May 16, 2023
kellen
kellen approved these changes May 16, 2023
View reviewed changes
Copy link
Contributor

@kellen kellen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add some more context in the PR description? Seems like the most salient change is adding these coder test syntaxes?

scio-test/src/test/scala/com/spotify/scio/coders/AvroCoderTest.scala Outdated
@@ -1,5 +1,5 @@
/*
* Copyright 2021 Spotify AB.
* Copyright 2019 Spotify AB.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a new file, but I just copy pasted that header from somewhere :)

scio-core/src/main/scala/com/spotify/scio/coders/Coder.scala
@@ -681,7 +321,7 @@ private[coders] object CoderStackTrace {
.currentThread()
.getStackTrace
.dropWhile(!_.getClassName.contains(CoderMaterializer.getClass.getName))
.take(10)
.take(15)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the reason for this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

stacktrace increased after adding syntax sugar to CoderAssertions, with 10 it cannot reach the CoderMaterializer.getClass.getName in tests, so impossible to verify it works. So totally driven by test case

scio-core/src/main/scala/com/spotify/scio/coders/instances/JodaCoders.scala
@@ -49,7 +49,7 @@ object JodaCoders {
}
}

final private class JodaDateTimeCoder extends AtomicCoder[DateTime] {
final private[coders] class JodaDateTimeCoder extends AtomicCoder[DateTime] {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the intent here that users will never create a coder instance and instead use the ones defined in JodaCoders and inherited by Coder?

Copy link
Contributor Author

@shnapz shnapz May 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the intent of the change - a scope widening to access it from tests :)
by design it was private, I assume, to incapsulate this complexity and deliver coders as implicits (consumed by macro-generated coders)

@shnapz
Copy link
Contributor Author

shnapz commented May 16, 2023

@kellen will add more context in the description

@shnapz
Copy link
Contributor Author

shnapz commented May 16, 2023

@RustedBones scalafix is fixed

@shnapz shnapz merged commit 62751fe into main May 18, 2023
@shnapz shnapz deleted the v0.13.x branch May 18, 2023 13:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kellen kellen kellen approved these changes

@RustedBones RustedBones RustedBones approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.13.0
Development

Successfully merging this pull request may close these issues.
3 participants
@shnapz @RustedBones @kellen