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.

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

Add preview test #97

Merged
merged 27 commits into from
Sep 15, 2022
Merged

Add preview test #97

merged 27 commits into from
Sep 15, 2022

Conversation

sobaya-0141
Copy link
Contributor

@sobaya-0141 sobaya-0141 commented Sep 2, 2022

}
}
}
}
Copy link

Choose a reason for hiding this comment

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

[spotless] reported by reviewdog 🐶

Suggested change
}
}

import com.airbnb.android.showkase.annotation.ShowkaseRootModule

@ShowkaseRoot
class ScreenshotShowkaseModule : ShowkaseRootModule
Copy link

Choose a reason for hiding this comment

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

[spotless] reported by reviewdog 🐶

Suggested change
class ScreenshotShowkaseModule : ShowkaseRootModule
class ScreenshotShowkaseModule : ShowkaseRootModule

@@ -29,6 +29,8 @@ import com.google.accompanist.pager.rememberPagerState
import io.github.droidkaigi.confsched2022.designsystem.theme.KaigiScaffold
import io.github.droidkaigi.confsched2022.designsystem.theme.KaigiTheme
import io.github.droidkaigi.confsched2022.feature.sessions.SessionsUiModel.ScheduleState.Loaded
import io.github.droidkaigi.confsched2022.feature.sessions.SessionsUiModel.ScheduleState.Loading
import io.github.droidkaigi.confsched2022.model.DroidKaigiSchedule
Copy link

Choose a reason for hiding this comment

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

[spotless] reported by reviewdog 🐶

Suggested change
import io.github.droidkaigi.confsched2022.model.DroidKaigiSchedule

@sobaya-0141
Copy link
Contributor Author

Related paparazzi Issues
cashapp/paparazzi#565
cashapp/paparazzi#489 (comment)

@takahirom
Copy link
Member

Please make sure that the addition of screen shots are not in the history due to the repository size. I think you can rebase and squash the commit. 🙏

import io.github.droidkaigi.confsched2022.model.Contributor
import io.github.droidkaigi.confsched2022.model.fakes
Copy link

Choose a reason for hiding this comment

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

[spotless] reported by reviewdog 🐶

Suggested change
import io.github.droidkaigi.confsched2022.model.Contributor
import io.github.droidkaigi.confsched2022.model.fakes

@github-actions
Copy link

github-actions bot commented Sep 5, 2022

There is a Diff in the screenshot

3 similar comments
@github-actions
Copy link

github-actions bot commented Sep 5, 2022

There is a Diff in the screenshot

@github-actions
Copy link

github-actions bot commented Sep 5, 2022

There is a Diff in the screenshot

@github-actions
Copy link

github-actions bot commented Sep 5, 2022

There is a Diff in the screenshot

@github-actions
Copy link

github-actions bot commented Sep 5, 2022

There are differences in Compose previews. Please check your build and download the diff artifact.
https://github.com/DroidKaigi/conference-app-2022/actions/workflows/ScreenShotTest.yml

# Conflicts:
#	feature-sessions/src/main/java/io/github/droidkaigi/confsched2022/feature/sessions/Sessions.kt
@sobaya-0141 sobaya-0141 marked this pull request as ready for review September 5, 2022 12:52
@sobaya-0141
Copy link
Contributor Author

Conflicts resolved.

@github-actions
Copy link

github-actions bot commented Sep 6, 2022

There are differences in Compose previews. Please check your build and download the diff artifact.
https://github.com/DroidKaigi/conference-app-2022/actions/workflows/ScreenShotTest.yml

with:
message: |
There are differences in Compose previews. Please check your build and download the diff artifact.
https://github.com/DroidKaigi/conference-app-2022/actions/workflows/ScreenShotTest.yml
Copy link

@hkusu hkusu Sep 6, 2022

Choose a reason for hiding this comment

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

It might be kinder to write https://github.com/${{ github.repository }}/actions/runs/${{ github.run_id }}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your comment.
I will fix it the way you taught me.

Comment on lines 3 to 4
branches:
- main
Copy link

@hkusu hkusu Sep 6, 2022

Choose a reason for hiding this comment

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

push:
  branches:
    - main

This may not be necessary, because it should work only at the time of pull request as it is only for pull requests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're absolutely right.

run: ./gradlew preview-screenshots:verifyPaparazziDebug

- name: Error Screenshot tests
continue-on-error: true
Copy link

Choose a reason for hiding this comment

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

This may not be necessary, because the verifyPaparazziDebug step specifies continue-on-error: true, so it will always succeed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I forgot to delete this one:sweat_smile:

path: preview-screenshots/out/failures

- name: Comment PR
continue-on-error: true
Copy link

Choose a reason for hiding this comment

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

This may not be necessary, too.

@github-actions
Copy link

github-actions bot commented Sep 8, 2022

There are differences in Compose previews. Please check your build and download the diff artifact.
https://github.com/DroidKaigi/conference-app-2022/actions/runs/3014753123

@sobaya-0141
Copy link
Contributor Author

Commit history has been reorganized and made available for review.

plugins {
id("droidkaigi.primitive.android")
id("droidkaigi.primitive.android.kotlin")
id("droidkaigi.primitive.android.compose")
id("droidkaigi.primitive.android.hilt")
id("droidkaigi.primitive.molecule")
id("droidkaigi.primitive.spotless")
alias(libs.plugins.ksp)
Copy link
Member

@takahirom takahirom Sep 9, 2022

Choose a reason for hiding this comment

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

Can you create "droidkaigi.primitive.android.compose.showkase"?

id("droidkaigi.primitive.molecule")
id("droidkaigi.primitive.spotless")
alias(libs.plugins.ksp)
alias(libs.plugins.paparazzi)
Copy link
Member

Choose a reason for hiding this comment

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

Can you create "droidkaigi.primitive.android.paparazzi"?

@@ -4,7 +4,7 @@ pluginManagement {
gradlePluginPortal()
google()
mavenCentral()
// maven("https://oss.sonatype.org/content/repositories/snapshots/")
maven("https://oss.sonatype.org/content/repositories/snapshots/")
Copy link
Member

Choose a reason for hiding this comment

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

If we make it a plugin, we'll be able to remove this? 👀

@takahirom
Copy link
Member

@sobaya-0141 I commented on some points 🙏 Could you please take a look at the comment?

@sobaya-0141
Copy link
Contributor Author

Thank you for your review.
We will respond to it now.
Please wait for a while.

run: mkdir -p ~/.gradle ; cp .github/ci-gradle.properties ~/.gradle/gradle.properties

- name: Set up JDK 11
uses: actions/setup-java@v1
Copy link
Contributor

Choose a reason for hiding this comment

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

[nit] Would you keep using v1 action? or just forget changing to v3?
I can see the v3.x.x in the action releases

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd like to know the your thought if you have. 🙇

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, you are right, it should be v3.x.x.:pray:

) {
NEXUS_5(DeviceConfig.NEXUS_5),
PIXEL_5(DeviceConfig.PIXEL_5),
PIXEL_C(DeviceConfig.PIXEL_C),
Copy link
Contributor

Choose a reason for hiding this comment

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

[nit] How about consider to adding the Pixel6 or Nexus 10 ? The Pixel6 is latest device, The Nexus 10 is a one of the tablet devices(I'm not sure we are suppressed to support fro Table devices)

Copy link
Contributor

@fumiya-kume fumiya-kume Sep 12, 2022

Choose a reason for hiding this comment

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

oh, sorry. I never know the Pixel C is tablet device...! bit old device than my engineer life...!

import com.google.testing.junit.testparameterinjector.TestParameter.TestParameterValuesProvider
import com.google.testing.junit.testparameterinjector.TestParameterInjector
import org.junit.Rule
import org.junit.Test
Copy link
Contributor

Choose a reason for hiding this comment

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

Just for confirmation 👀 , is Paparazzi possible to working with JUnit5? 🤔 if possible, other engineer can use modern toolchain...! ✨

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't check this one.
I would like to get it working first.:pray:


steps:
- uses: actions/checkout@v3
- uses: actions/setup-java@v1
Copy link
Contributor

Choose a reason for hiding this comment

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

@takahirom
Copy link
Member

I fixed the conflict in GitHub.

@takahirom
Copy link
Member

@sobaya-0141 If you don't have time, we can develop this PR by recruiting contributors to this PR. What do you think?

@sobaya-0141
Copy link
Contributor Author

@takahirom
Let me do it until 10:00 today.
If I cannot complete it by then, please assign it to someone else.:bow:

@sobaya-0141
Copy link
Contributor Author

I can't seem to find the time, so let me give it up.:bow::bow::bow:

@takahirom takahirom mentioned this pull request Sep 14, 2022
@aqua-ix aqua-ix mentioned this pull request Sep 15, 2022
@github-actions github-actions bot temporarily deployed to deploygate-distribution September 15, 2022 02:24 Inactive
@takahirom
Copy link
Member

Let's merge!

@takahirom takahirom merged commit 6ec4422 into main Sep 15, 2022
@takahirom takahirom deleted the add_preview_test branch September 15, 2022 04:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants