-
Notifications
You must be signed in to change notification settings - Fork 52
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
Ignores RestorePurchasesDialog
previews using IgnoreEmergeSnapshot
#2116
Conversation
ui/revenuecatui/build.gradle
Outdated
@@ -56,6 +56,8 @@ android { | |||
} | |||
|
|||
dependencies { | |||
compileOnly libs.emerge.snapshots.annotations |
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.
Does this make sense at all? I see it's making the tests fail to compile
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.
Hmm in the docs, it seems they say to use debugImplementation
... but that's weird, since it wouldn't work on release I guess? https://docs.emergetools.com/docs/excluding-previews
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.
Oh interesting, I stopped at "You'll need to include this as an implementation
dependency in whatever soruce set you're intending to ignore snapshots." lol
Yeah, with debug, it won't be able to compile on release
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.
It says "Previews are only included in debug
builds". Maybe it just magically works
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.
weird, as soon as I add it
Caused by: com.vanniktech.maven.publish.MissingVariantException: Invalid MavenPublish Configuration. Unable to find variant to publish named defaultsRelease. Try setting the 'androidVariantToPublish' property in the mavenPublish extension object to something that matches the variant that ought to be published.
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've thought of copying the annotation under the same package to our repo? Is that too crazy?
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.
What's the error when using compileOnly
? I think if we use compileOnly
in revenuecatui
, we should also add it as debugImplementation
to the testpurchasesuiandroidcompatibility
app.
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.
Doing that leads to this, which is very strange to me:
Caused by: com.vanniktech.maven.publish.MissingVariantException: Invalid MavenPublish Configuration. Unable to find variant to publish named defaultsRelease. Try setting the 'androidVariantToPublish' property in the mavenPublish extension object to something that matches the variant that ought to be published.
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 think that's just covering the real issue
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.
for the future, the error was stupid and I had not commited the change to the toml file
📸 Snapshot Test189 unchanged
🛸 Powered by Emerge Tools |
This reverts commit 9d098fd.
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.
Let's go! This will finally turn main
green again 😄
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2116 +/- ##
=======================================
Coverage 80.89% 80.89%
=======================================
Files 272 272
Lines 9047 9047
Branches 1276 1276
=======================================
Hits 7319 7319
Misses 1193 1193
Partials 535 535 ☔ View full report in Codecov by Sentry. |
There's an issue in Emerge Snapshots when previewing a Dialog EmergeTools/emerge-android#136
They've suggested we ignore the snapshot for now. Added the dependency as
compileOnly
so it doesn't get included in the aar