8000 [#439] Implement BDD style alias methods by jameswhitehead4 · Pull Request #662 · mockk/mockk · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

[#439] Implement BDD style alias methods #662

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

Closed
wants to merge 1 commit into from
Closed

[#439] Implement BDD style alias methods #662

wants to merge 1 commit into from

Conversation

jameswhitehead4
Copy link

No description provided.

@oleksiyp
Copy link
Collaborator
oleksiyp commented Jul 9, 2021

Hi, thank you for your contribution.

Such high-level definitions should be very carefully considered.
As once done not possible to revert back.

What should be checked IMHO:

  1. It might be clashing with other libraries
  2. It might be not what really mocking is suppose to be.
  3. It might be not that universal (i.e. applicable only for the example provided)
  4. It is possible always to easily define such aliases in a particular project
  5. Generally should be considered if it is desirable by the community.

@jamesxwhitehead can you provide more examples of its usage.
@Raibaz it would be good to go through the list and somehow prove that this is a viable change(maybe through more and more examples and counterexamples)

@oleksiyp
Copy link
Collaborator
oleksiyp commented Jul 9, 2021

@igorwojda maybe you also have more examples

@jameswhitehead4
Copy link
Author

Hi @oleksiyp

I definitely agree with your points. I implemented this feature as it is tagged as "good-first-issue" and seemed like one I could tackle. This is my very first contribution to open-source 😃 If you do not feel that this is a good feature for MockK that's definitely fine, and perhaps you could point me towards a different issue that you would like to see implemented.

8000
@oleksiyp
Copy link
Collaborator
oleksiyp commented Jul 9, 2021

No, I am not saying it shouldn't be considered. I just say let's slow down a bit and think, bring rational arguments... again thanks! Especially for your first contribution!!!

@hrach
Copy link
Contributor
hrach commented Jul 9, 2021

TBH introduction of such aliases seems rather dangerous to me. It is not a problem in small teams but ours it is rather a bigger one and we would have to put non-trivial work to:

  • teach everybody (and newcomers) what style we use (and why)
  • validate that the proper style is used - i.e. write custom lints not to let it pass in CI

Since these are pure aliases I'd guess that they can nicely live as an extension library or directly in your code-base.

@Raibaz
Copy link
Collaborator
Raibaz commented Jul 9, 2021

First of all, thanks a lot @jamesxwhitehead for sending this through.

However, I agree with @hrach's point that having two different ways of doing things may introduce ambiguity and, as @oleksiyp mentions, this introduction should be carefully considered.

Perhaps we could implement these as an additional module, like mockk-bdd, to make sure that using these aliases requires adding an additional dependency and being intentional in adding them to a project.

Then again, the last point in @oleksiyp's list is what concerns me most: do we really need these aliases that basically just rename MockK's functions without introducing any additional feature?

I personally don't think we do, but I'd like to gather some community feedback on this.

Also, @jamesxwhitehead, sorry for making the issue look easier than it actually was :(

@igorwojda
Copy link
igorwojda commented Jul 9, 2021

I think that adding these styles to the project could create some issues. The biggest one IMO is the potential usages of both styles in a single project (leading to code inconsistency).

I think we would have a wider look ad check what the industry is doing:

  1. Testing frameworks seem to have multiple testing styles

Heaving various styles could be beneficial because of personal or business preferences (assuming the business is using BDD everywhere else in the company), however, we should be explicit about using them to avoid style mixing.

I think we should follow @Raibaz suggestion to create separate dependencies each with its own style eg.

androidTestImplementation "io.mockk:mockk:{version}"
androidTestImplementation "io.mockk:mockk-bdd:{version}"

androidTestImplementation "io.mockk:mockk-android:{version}"
androidTestImplementation "io.mockk:mockk-android-bdd:{version}"

What should be checked IMHO:

  1. It might be clashing with other libraries

Nothing comes to my mind that could potentially conflict

  1. It might be not what really mocking is suppose to be.

We have BDDMockito after all.

  1. It might be not that universal (i.e. applicable only for the example provided)

This must be closely considered

  1. It is possible always to easily define such aliases in a particular project

I guess the problem with aliases as oppose to separate in is that you can mix the styles. It also affects error messages, right?

  1. Generally should be considered if it is desirable by the community.

My original BDD issue has many likes #439 and the existence of BDDMockito is somehow confirmed as well

@igorwojda
Copy link

BTW Great contribution @jamesxwhitehead 🎉

@oleksiyp
Copy link
Collaborator
oleksiyp commented Jul 9, 2021

Yes, separate module seems to be the most reasonable way to go

@jameswhitehead4
Copy link
Author

I agree 100% with everything above. I will try to re-implement this feature as a new module instead and we can go from there 😄 Thanks for the great feedback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants
0