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

Issue/8676 scmainvm tests #9142

Merged
merged 10 commits into from
Feb 7, 2019
Merged

Issue/8676 scmainvm tests #9142

merged 10 commits into from
Feb 7, 2019

Conversation

malinajirka
Copy link
Contributor

Fixes #8676

  • Adds units tests for NewSiteCreationMainVM
  • Introduces NewSiteCreationStepsProvider so we can mock it in tests
  • Adds observeForever() to SingleEventObservable so we can easily listen for changes in tests (regular observer() requires a LifecycleOwner)

To test:

  • set the feature flag NEW_SITE_CREATION_ENABLED in build.gradle to true
  • test the new site creation flow -> make sure all the screens are shown in the correct order -> Segments, Verticals, SiteInfo, Domain, SitePreview

Update release notes:

  • [ x ] If there are user facing changes, I have added an item to RELEASE-NOTES.txt.

Copy link
Contributor

@oguzkocer oguzkocer left a comment

Choose a reason for hiding this comment

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

@malinajirka I am done with my first review. I've left some suggestions, most of which is for AS warnings, let me know what you think of them!

P.S: There are some things in this PR that makes me really uncomfortable and I really would like to make some suggestions to improve them, but it feels impossible in the JVM. Especially the siteCreationStateRestored test is full of assumptions, but I can't think of a way to improve that except for maybe extracting that logic in the VM to a public function with @VisibleForTesting annotation. That doesn't make me feel better either, so I am letting it go for now. I'll think on it, but I don't expect to actually find a good solution to them.

@malinajirka
Copy link
Contributor Author

Thanks @oguzkocer !

  1. AS issues - I was using a wrong AS inspection profile. (issues fixed in fdf52bb)

  2. I've added WizardManager to Dagger graph in 4bb181c so we can mock it in unit tests.

  3. I've made a lot of changes to the tests in 94a894b. All the methods should be a bit more readable and more reliable. Except of siteCreationStateRestored and siteCreationStepIndexRestored:( - I'm still not sure what's the best approach to testing state restoration.

Copy link
Contributor

@oguzkocer oguzkocer left a comment

Choose a reason for hiding this comment

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

Thanks a lot for all the improvements @malinajirka, I really love the new tests! They feel much more like unit tests now as most of them are just verifying a small bit of logic.

I'm still not sure what's the best approach to testing state restoration.

Me neither. Maybe we want to remove the writeToBundle and savedInstanceState logic from the VM and have it in the Activity instead. I am not sure how I feel about it, but it'd certainly make the tests more meaningful. I am also totally fine with keeping it as is. It's your call :)

@Test
fun siteCreationStateRestored() {
val expectedState = SiteCreationState()
whenever(savedInstanceState.getParcelable<SiteCreationState>("key_site_creation_state"))
Copy link
Contributor

Choose a reason for hiding this comment

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

It'd be great if we can use the current constants in NewSiteCreationMainVM for these strings to avoid tests going out of sync with the VM. I believe making those public is the lesser of two evils here and there is a big risk with having them public. What do you think?

Note that this is for key_site_creation_state, key_current_step and key_site_creation_state string values. I am keeping all this to a single comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, I was thinking about this for some time. I miss "package" scope so much :(. The reason why I felt like it's better to hardcode it in tests is that having them public "pollutes" autocomplete and we can't push anything to the develop without a working unit tests anyway. However, I don't feel strongly about it - updated in 464757c

@malinajirka
Copy link
Contributor Author

Thanks @oguzkocer. I've updated the last two minor issues.

Me neither. Maybe we want to remove the writeToBundle and savedInstanceState logic from the VM and have it in the Activity instead. I am not sure how I feel about it, but it'd certainly make the tests more meaningful. I am also totally fine with keeping it as is. It's your call :)

Hmm I realize these tests are far from perfect. But I think it's still better to have these tests which at least try to test whether the state and step_index is restored than don't have any tests at all. Lets just think about it for some time and update it later if we come up with a better approach.

Copy link
Contributor

@oguzkocer oguzkocer left a comment

Choose a reason for hiding this comment

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

Thanks for all the updates @malinajirka! Looks good :shipit:

@oguzkocer oguzkocer merged commit 69da984 into develop Feb 7, 2019
@oguzkocer oguzkocer deleted the issue/8676-scmainvm-tests branch February 7, 2019 16:30
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