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

Fix #3422: Separate MarkTopicsCompletedActivityTest into activity and fragment test files #3471

Merged
merged 6 commits into from
Jul 26, 2021
Merged

Fix #3422: Separate MarkTopicsCompletedActivityTest into activity and fragment test files #3471

merged 6 commits into from
Jul 26, 2021

Conversation

yashraj-01
Copy link
Contributor

@yashraj-01 yashraj-01 commented Jul 13, 2021 •
edited

Explanation

Fixes #3422. Separated MarkTopicsCompletedActivityTest into activity and fragment test files.

Screenshots

  • Expresso test results for MarkTopicsCompletedActivityTest
    Screenshot from 2021-07-17 17-43-18

  • Expresso test results for MarkTopicsCompletedFrgmentTest
    Screenshot from 2021-07-17 17-26-03

Checklist

  • The PR title starts with "Fix #bugnum: ", followed by a short, clear summary of the changes. (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
  • The PR explanation includes the words "Fixes #bugnum: ..." (or "Fixes part of #bugnum" if the PR only partially fixes an issue).
  • The PR follows the style guide.
  • The PR does not contain any unnecessary auto-generated code from Android Studio.
  • The PR is made from a branch that's not called "develop".
  • The PR is made from a branch that is up-to-date with "develop".
  • The PR's branch is based on "develop" and not on any other branch.
  • The PR is assigned to an appropriate reviewer in both the Assignees and the Reviewers sections.

@github-actions
Copy link

Hi! @yashraj-01 Welcome to Oppia! Could you please follow the instructions here and sign the CLA Sheet to get started? You'll need to do this before we can accept your PR. Thanks!

BenHenning
BenHenning reviewed Jul 13, 2021
View reviewed changes
Copy link
Sponsor Member

@BenHenning BenHenning left a comment

Choose a reason for hiding this comment

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

Nice idea to ensure test coverage. Had a few comments--PTAL.

app/src/sharedTest/java/org/oppia/android/app/devoptions/MarkTopicsCompletedFragmentTest.kt Outdated Show resolved Hide resolved
app/src/sharedTest/java/org/oppia/android/app/devoptions/MarkTopicsCompletedActivityTest.kt Outdated Show resolved Hide resolved
@BenHenning BenHenning assigned yashraj-01 and unassigned BenHenning Jul 13, 2021
@rt4914
Copy link
Contributor

rt4914 commented Jul 14, 2021

Please add screenshot showing that Espresso Tests are passing for the files modified in this PR. Also once @BenHenning 's comments are resolved and fixed just ping me so that I can check it accordingly.

1 similar comment
@rt4914
Copy link
Contributor

rt4914 commented Jul 14, 2021

Please add screenshot showing that Espresso Tests are passing for the files modified in this PR. Also once @BenHenning 's comments are resolved and fixed just ping me so that I can check it accordingly.

@yashraj-01 yashraj-01 assigned BenHenning and unassigned yashraj-01 Jul 14, 2021
@BenHenning BenHenning assigned yashraj-01 and unassigned BenHenning Jul 16, 2021
@yashraj-01 yashraj-01 assigned BenHenning and unassigned yashraj-01 Jul 17, 2021
@yashraj-01 yashraj-01 mentioned this pull request Jul 18, 2021
Merged
8 tasks
rt4914
rt4914 approved these changes Jul 19, 2021
View reviewed changes
Copy link
Contributor

@rt4914 rt4914 left a comment

Choose a reason for hiding this comment

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

LGTM.

@rt4914 rt4914 removed their assignment Jul 19, 2021
BenHenning
BenHenning reviewed Jul 20, 2021
View reviewed changes
Copy link
Sponsor Member

@BenHenning BenHenning left a comment

Choose a reason for hiding this comment

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

Thanks @yashraj-01! Left some more thoughts, but this in general looks good.

.../oppia/android/app/devoptions/marktopicscompleted/testing/MarkTopicsCompletedTestActivity.kt Outdated Show resolved Hide resolved
.../oppia/android/app/devoptions/marktopicscompleted/testing/MarkTopicsCompletedTestActivity.kt Show resolved Hide resolved
app/src/sharedTest/java/org/oppia/android/app/devoptions/MarkTopicsCompletedActivityTest.kt Outdated Show resolved Hide resolved
@BenHenning BenHenning assigned yashraj-01 and unassigned BenHenning Jul 20, 2021
@yashraj-01 yashraj-01 assigned BenHenning and unassigned yashraj-01 Jul 20, 2021
@yashraj-01
Copy link
Contributor Author

@BenHenning Bazel build is successful locally but it is failing here. Is this an issue with the code or the workflow?
Screenshot from 2021-07-20 14-58-28

@BenHenning
Copy link
Sponsor Member

Hmm I'm not sure @yashraj-01. It might be a weird transient error or potentially a flake, but I've never seen the dex step fail specifically in CI before. I'm restarting the workflow to see if that helps.

In the meantime, I responded to the open comment thread above. PTAL & reassign to me once that thread is addressed & the latest CI flow has finished.

@BenHenning BenHenning assigned yashraj-01 and unassigned BenHenning Jul 21, 2021
@yashraj-01
Copy link
Contributor Author

Hmm I'm not sure @yashraj-01. It might be a weird transient error or potentially a flake, but I've never seen the dex step fail specifically in CI before. I'm restarting the workflow to see if that helps.

In the meantime, I responded to the open comment thread above. PTAL & reassign to me once that thread is addressed & the latest CI flow has finished.

Actually, this issue is also faced by @Arjupta and @FareesHussain. So, I think there might be some issue with the CI.

@oppiabot oppiabot bot assigned Arjupta and FareesHussain and unassigned yashraj-01 Jul 21, 2021
@oppiabot
Copy link

oppiabot bot commented Jul 21, 2021

Unassigning @yashraj-01 since a re-review was requested. @yashraj-01, please make sure you have addressed all review comments. Thanks!

@yashraj-01
Copy link
Contributor Author

Hmm I'm not sure @yashraj-01. It might be a weird transient error or potentially a flake, but I've never seen the dex step fail specifically in CI before. I'm restarting the workflow to see if that helps.

In the meantime, I responded to the open comment thread above. PTAL & reassign to me once that thread is addressed & the latest CI flow has finished.

@BenHenning I have addressed the comment. Also, the CI check is still failing with the same dex error.

BenHenning
BenHenning approved these changes Jul 22, 2021
View reviewed changes
Copy link
Sponsor Member

@BenHenning BenHenning left a comment

Choose a reason for hiding this comment

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

Thanks @yashraj-01. LGTM since the CI failure should be fixed with #3513.

@BenHenning BenHenning assigned yashraj-01 and unassigned BenHenning Jul 22, 2021
@oppiabot oppiabot bot added the PR: LGTM label Jul 22, 2021
@yashraj-01
Copy link
Contributor Author

Merged with latest develop. Also, assigning @Sarthak2601 for review as the mentor.

@yashraj-01 yashraj-01 assigned Sarthak2601 and unassigned yashraj-01 Jul 22, 2021
@yashraj-01 yashraj-01 mentioned this pull request Jul 22, 2021
Merged
8 tasks
This was referenced Jul 22, 2021
Merged
Merged
Merged
Sarthak2601
Sarthak2601 approved these changes Jul 26, 2021
View reviewed changes
Copy link
Contributor

@Sarthak2601 Sarthak2601 left a comment

Choose a reason for hiding this comment

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

LGTM.

@Sarthak2601 Sarthak2601 merged commit c42dca3 into oppia:develop Jul 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rt4914 rt4914 rt4914 approved these changes

@BenHenning BenHenning BenHenning approved these changes

@Sarthak2601 Sarthak2601 Sarthak2601 approved these changes

Assignees

@Sarthak2601 Sarthak2601

Labels
PR: LGTM
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Separate MarkTopicsCompletedActivityTest into activity and fragment test files
6 participants
@yashraj-01 @rt4914 @BenHenning @Sarthak2601 @FareesHussain @Arjupta