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

Added github actions to run uiTests #1048

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

epicadk
Copy link
Member

@epicadk epicadk commented Dec 28, 2020

Description

Include a summary of the change and relevant motivation/context. List any dependencies that are required for this change.

Fixes #168

Type of Change:

Delete irrelevant options.

  • Quality Assurance

Code/Quality Assurance Only

  • New feature (non-breaking change which adds functionality pre-approved by mentors)

How Has This Been Tested?

Describe the tests you ran to verify your changes. Provide instructions or GIFs so we can reproduce. List any relevant details for your test.

Checklist:

Delete irrelevant options.

  • My PR follows the style guidelines of this project
  • I have performed a self-review of my own code or materials
  • I have commented my code or provided relevant documentation, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • Any dependent changes have been merged
  • I have written Kotlin Docs whenever is applicable

Code/Quality Assurance Only

  • My changes generate no new warnings
  • My PR currently breaks something (fix or feature that would cause existing functionality to not work as expected)
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been published in downstream modules

@epicadk epicadk force-pushed the uitests_ga branch 2 times, most recently from 4249e80 to 144c01e Compare December 28, 2020 18:52
@epicadk epicadk marked this pull request as ready for review December 28, 2020 19:04
@epicadk
Copy link
Member Author

epicadk commented Dec 28, 2020

@isabelcosta done can you review ? The tests are failing because the existing tests fail.

@isabelcosta
Copy link
Member

@epicadk is it possible to have this as a separate check on GitHub checks 🤔 to separate build from tests so we know right aways why something is failing. Would you know how to do that?

@epicadk
Copy link
Member Author

epicadk commented Dec 28, 2020

@epicadk is it possible to have this as a separate check on GitHub checks 🤔 to separate build from tests so we know right aways why something is failing. Would you know how to do that?

Hmm it is but we can also check the details to view which test has failed , and it would also avoid setting up of another Ubuntu system.

Let me know how I should proceed.

@epicadk epicadk force-pushed the uitests_ga branch 5 times, most recently from 0ff04a8 to 6d8a1e2 Compare December 29, 2020 09:22
@epicadk
Copy link
Member Author

epicadk commented Dec 29, 2020

@epicadk is it possible to have this as a separate check on GitHub checks 🤔 to separate build from tests so we know right aways why something is failing. Would you know how to do that?

Hmm it is but we can also check the details to view which test has failed , and it would also avoid setting up of another Ubuntu system.

Let me know how I should proceed.

@isabelcosta I created a separate workflow using mac-os ( as that is what was recommended in the docs here) I think it is working as expected now please review.

Copy link
Member

@isabelcosta isabelcosta left a comment

Choose a reason for hiding this comment

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

I am not very aware of how tests are run in Android, but this looks good to me. I saw the additional PR check and I think that looks good separate from the build workflow. I will ask for more reviews on this from the community.

Thank you so much for working on this @epicadk 🙌

Copy link
Contributor

@annabauza annabauza left a comment

Choose a reason for hiding this comment

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

Suggested minor changes to observe pas and pushes to master branch as well.

.github/workflows/UI tests.yml Outdated Show resolved Hide resolved
@annabauza
Copy link
Contributor

Prs not pas

@isabelcosta isabelcosta added the Status: Needs Review PR needs an additional review or a maintainer's review. label Dec 29, 2020
@vj-codes vj-codes added Status: Changes Requested Changes are required to be done by the PR author. and removed Status: Needs Review PR needs an additional review or a maintainer's review. labels Dec 30, 2020
Copy link
Contributor

@annabauza annabauza left a comment

Choose a reason for hiding this comment

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

As in comments

@epicadk
Copy link
Member Author

epicadk commented Dec 31, 2020

I've made the requested changes @isabelcosta @annabauza. : )

annabauza
annabauza previously approved these changes Dec 31, 2020
Copy link
Contributor

@annabauza annabauza left a comment

Choose a reason for hiding this comment

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

💯

@vj-codes vj-codes added Status: Needs Review PR needs an additional review or a maintainer's review. and removed Status: Changes Requested Changes are required to be done by the PR author. labels Jan 2, 2021
@epicadk epicadk requested a review from annabauza January 6, 2021 06:48
@epicadk
Copy link
Member Author

epicadk commented Jan 6, 2021

I just removed the comments in the yml file. Should I squash the commits?

@isabelcosta
Copy link
Member

I just removed the comments in the yml file. Should I squash the commits?

No worries. No need for that, I will squash anyways when merging @epicadk :) thank you so much for working on this one 🙌

annabauza
annabauza previously approved these changes Jan 23, 2021
Copy link
Contributor

@annabauza annabauza left a comment

Choose a reason for hiding this comment

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

💯

@isabelcosta
Copy link
Member

@epicadk could you please update this branch, not sure if the tests are already fixed 🤔 and if they are not yet fixed, we have to pin community again to help us fix the tests

@epicadk
Copy link
Member Author

epicadk commented Feb 19, 2021

@epicadk could you please update this branch, not sure if the tests are already fixed 🤔 and if they are not yet fixed, we have to pin community again to help us fix the tests

Ran the tests locally and they haven't been fixed yet. I'll look into this.

Also I had brought this up in a session but sometimes the sessions times out and that is why the tests fail.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Needs Review PR needs an additional review or a maintainer's review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add run UI test step to GitHub Action
4 participants