-
Notifications
You must be signed in to change notification settings - Fork 514
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.
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
improve testing framework #4962
Conversation
@@ -36,15 +37,15 @@ trait PipelineTestUtils { | |||
* } | |||
* }}} | |||
*/ | |||
def runWithContext[T](fn: ScioContext => T): ScioExecutionContext = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is breaking. type parameter was not really useful here
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #4962 +/- ##
==========================================
- Coverage 63.42% 63.37% -0.06%
==========================================
Files 288 291 +3
Lines 10805 10841 +36
Branches 778 781 +3
==========================================
+ Hits 6853 6870 +17
- Misses 3952 3971 +19 ☔ View full report in Codecov by Sentry. |
3b481e3
to
434d558
Compare
scio-test/src/main/scala/com/spotify/scio/testing/JobTest.scala
Outdated
Show resolved
Hide resolved
scio-test/src/main/scala/com/spotify/scio/testing/JobTest.scala
Outdated
Show resolved
Hide resolved
scio-smb/src/main/scala/com/spotify/scio/smb/syntax/SortMergeBucketScioContextSyntax.scala
Outdated
Show resolved
Hide resolved
scio-smb/src/test/scala/com/spotify/scio/smb/SortMergeBucketTest.scala
Outdated
Show resolved
Hide resolved
e051fc5
to
ae86acf
Compare
In order to make testing work with |
99e71be
to
ad0e31d
Compare
scio-test/src/main/scala/com/spotify/scio/testing/RunEnforcementJobTest.scala
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work!
* } | ||
* }}} | ||
*/ | ||
def runWithOverrides( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this instead be chained onto ScioExecutionContext
so we could avoid the distinct naming? e.g.
runWithContext { sc =>
// ...
}
.withOverrides(
TransformOverride.of("operation", (v: Int) => v.toString)
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried but then it's not possible to decide when the pipeline should be run.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh yeah that makes sense. Could we nest it somehow?
runWithContext { sc =>
sc.withOverrides(
TransformOverride.of(...)
)
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks nice and should be possible. Will try that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change is a bit risky because the transform must be defined before the withOverrides
call or we need to store the override and call replaceAll
just before run.
I tried to integrate the TestDataManager
here but it is inconvenient
e8e36c2
to
c19f88c
Compare
c19f88c
to
987f64d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🙌
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
amazing work!
scio-smb/src/test/scala/com/spotify/scio/smb/SortMergeBucketTest.scala
Outdated
Show resolved
Hide resolved
import com.spotify.scio.io.{KeyedIO, TapOf, TapT, TestIO} | ||
import com.spotify.scio.util.ScioUtil | ||
|
||
final class SortedBucketIO[K, T](path: String, override val keyBy: T => K)(implicit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
now that I think about it, this class name is a bit confusing as it conflicts with the beam packaged SortedBucketIO https://github.com/spotify/scio/blob/main/scio-smb/src/main/java/org/apache/beam/sdk/extensions/smb/SortedBucketIO.java. I was testing out a custom SortedBucketIO.Read override, but since I had import com.spotify.scio.smb._
in my job (in order to get the Scala API bindings for reads/writes), I had to rename the org.apache.beam.sdk.extensions.smb.SortedBucketIO
import to be able to use it
Maybe SmbIO
? SortedBucketTestIO
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I noticed that. I tried to keep consistency with java naming but It's probably to better to avoid conflict with SmbIO
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cool. I can make that change today!
Fix #4963
runWitnOverrides
to run pipeline unit-test, replacing some partsJobTest
io support for SMBJobTest
API for anonymous jobs (defined from context)