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

Add Mox.get_executed_calls/1 #140

Closed
wants to merge 2 commits into from
Closed

Conversation

whatyouhide
Copy link
Collaborator

Kicking off some code for #133.

I have no clue what I'm doing, I'm not familiar with Mox's codebase so I'm just sort of winging it.

@@ -812,7 +817,7 @@ defmodule Mox do
def __dispatch__(mock, name, arity, args) do
all_callers = [self() | caller_pids()]

case Mox.Server.fetch_fun_to_dispatch(all_callers, {mock, name, arity}) do
case Mox.Server.fetch_fun_to_dispatch(all_callers, {mock, name, arity}, args) do
Copy link
Member

Choose a reason for hiding this comment

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

This means we are copying all argument data to a separate process. :(

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Right, is that an issue in tests? We would do the same when using the send/2 trick to send calls to the parent process.

Any ideas on how to do this without copying the args?

Copy link
Member

Choose a reason for hiding this comment

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

the problem is that this enables this for everything by default. We would need to store it somewhere if we want to collect or not.

@@ -763,6 +763,11 @@ defmodule Mox do
verify_mock_or_all!(self(), mock, :test)
end

def get_executed_calls(mock) do
validate_mock!(mock)
Mox.Server.get_executed_calls(mock)
Copy link
Member

Choose a reason for hiding this comment

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

Also, is this safe to execute concurrently? It doesn't look like it is the case. :(

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It definitely isn't 😉 Will have to figure it out with the allowance.

@whatyouhide
Copy link
Collaborator Author

@josevalim this is just to get the ball rolling. Are we interested in this feature? If so, we can try to hash out the things that are borked 🥦

@whatyouhide
Copy link
Collaborator Author

Yeah... I give up on this, it's complex and I don't have the time just right now 😄 For now, the send(...) solution will have to do 😉

@whatyouhide whatyouhide deleted the al/get-executed-calls branch July 26, 2023 15:21
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.

2 participants