Skip to content

Add unit tests for PageListItemLabelsUseCase - #11281

Merged
jd-alexander merged 4 commits into
feature/master-pages-offline-supportfrom
unit-tests-for-page-labels
Feb 12, 2020
Merged

Add unit tests for PageListItemLabelsUseCase#11281
jd-alexander merged 4 commits into
feature/master-pages-offline-supportfrom
unit-tests-for-page-labels

Conversation

@malinajirka

Copy link
Copy Markdown
Contributor

Adds unit tests for #11278

Review instruction

  1. Review the code
  2. Wait until Issue/11128 page label colors #11280 gets merged
  3. Change target branch to feature/master-pages-offline-support
  4. Remove the "Not ready for merge" label
  5. Merge the PR

PR submission checklist:

  • I have considered adding unit tests where possible.
  • I have considered adding accessibility improvements for my changes.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@peril-wordpress-mobile

peril-wordpress-mobile Bot commented Feb 11, 2020

Copy link
Copy Markdown

You can test the changes on this Pull Request by downloading the APK here.

@jd-alexander jd-alexander self-assigned this Feb 11, 2020

@jd-alexander jd-alexander left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hi @malinajirka great test suite! It works as intended! Based on the comments I made on the page item labels PR, this one might get updated with more tests depending on your thoughts. So once you respond about to this, we can move forward here.

@malinajirka

Copy link
Copy Markdown
Contributor Author

It works as intended! Based on the comments I made on the page item labels PR, this one might get updated with more tests depending on your thoughts. So once you respond about to this, we can move forward here.

Tests updated in e8ce3a0. I decided to replace the static access to UploadUtils with an injectable wrapper. These tests are supposed to be unit test testing only CreatePageListItemLabelsUseCase I decided to remove some of the tests.

I created a ticket #11285 - for adding unit tests for the UploadUtils method.

It's ready for another round, thanks!

@jd-alexander jd-alexander left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for these changes @malinajirka! Looks good!

For some reason when I am running the test locally I am getting this exception.

org.mockito.exceptions.base.MockitoException: 
Mockito cannot mock this class: class org.wordpress.android.ui.uploads.UploadUtilsWrapper.
Can not mock final classes with the following settings :
 - explicit serialization (e.g. withSettings().serializable())
 - extra interfaces (e.g. withSettings().extraInterfaces(...))

You are seeing this disclaimer because Mockito is configured to create inlined mocks.
You can learn about inline mocks and their limitations under item #39 of the Mockito class javadoc.

I am going to look into my setup to see if it might be causing the issue; possibly the JDK. Will merge these changes since the test are passing fine 🚢

@jd-alexander
jd-alexander changed the base branch from issue/11128-page-label-colors to feature/master-pages-offline-support February 12, 2020 23:09
@jd-alexander
jd-alexander merged commit 29b1957 into feature/master-pages-offline-support Feb 12, 2020
@jd-alexander
jd-alexander deleted the unit-tests-for-page-labels branch February 12, 2020 23:10
@malinajirka

Copy link
Copy Markdown
Contributor Author

For some reason when I am running the test locally I am getting this exception.

That's quite weird. I have no idea what might be causing this issue. I've double checked it and I don't see such exception on my end.

@malinajirka malinajirka modified the milestones: 14.3, 14.6 Mar 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants