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.

Sign up for GitHub

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

Use ScopedValue to fix async issue #112

Merged
merged 10 commits into from
Jul 13, 2024
Merged

Use ScopedValue to fix async issue #112

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
ba6f41b
Use ContextVariablesX to fix async issue
omus Oct 6, 2021
230e310
Use ScopedValues rather than ContextVariables
oxinabox Feb 26, 2024
3048ff2
fixes (squash me)
oxinabox Mar 6, 2024
9f4d9ab
Apply suggestions from code review
oxinabox Mar 13, 2024
613ea94
Merge branch 'master' into ox/scoped_variables
omus Jul 11, 2024
020471c
Merge branch 'master' into ox/scoped_variables
omus Jul 13, 2024
54db84b
Define missing PATCH_ENV
omus Jul 13, 2024
90aaaad
Refactor tasks testset into async scope
omus Jul 13, 2024
73cb85d
Import ScopedValue alongside other imports
omus Jul 13, 2024
372e44b
Use VERSION check instead
omus Jul 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/deprecated.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,3 +12,5 @@ function enable(; force=false)
depwarn("`$m.enable(; force=$force)` is deprecated, use `$m.activate()` instead.", :enable)
activate()
oxinabox marked this conversation as resolved.
Show resolved Hide resolved
end

@deprecate set_active_env(f, pe) with_active_env(f, pe) false
omus marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

This isn't a proper deprecation as set_active_env would have it's changes persist beyond the scope of f. I think we should probably just make this a breaking change

Copy link
Member Author

Choose a reason for hiding this comment

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

fair, though this is what the previous change did, modulo also renaming set_active_env.
That function isn't actually exported.
Since the normal way to do it is to use apply(pe) do.
More an internal implementation detail of apply ?

Copy link
Member

Choose a reason for hiding this comment

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

I think I'm okay with stating that set_active_env and get_active_env are internal functions and don't require deprecations. Making this a breaking change would be the safest but I could be convinced to have this be non-breaking.

Copy link
Member Author

Choose a reason for hiding this comment

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

If it breaks something can always revert, tag patch, and rerelease as breaking

Copy link
Member Author

Choose a reason for hiding this comment

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

I will just delete them both and not deprecate

2 changes: 1 addition & 1 deletion src/mock.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ function get_alternate(pe::PatchEnv, target, args...)
end
end

get_alternate(target, args...) = get_alternate(get_active_env(), target, args...)
get_alternate(target, args...) = get_alternate(PATCH_ENV[], target, args...)

function _debug_msg(method::Union{Method,Nothing}, target, args)
call = "$target($(join(map(arg -> "::$(Core.Typeof(arg))", args), ", ")))"
Expand Down
27 changes: 17 additions & 10 deletions src/patch.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,19 +126,26 @@ end
```
"""
function apply(body::Function, pe::PatchEnv)
prev_pe = get_active_env()
set_active_env(merge(prev_pe, pe))
try
return body()
finally
set_active_env(prev_pe)
end
merged_pe = merge(PATCH_ENV[], pe)
return with_active_env(body, merged_pe)
end

function apply(body::Function, patches; debug::Bool=false)
return apply(body, PatchEnv(patches, debug))
end

const PATCH_ENV = Ref{PatchEnv}(PatchEnv())
set_active_env(pe::PatchEnv) = (PATCH_ENV[] = pe)
get_active_env() = PATCH_ENV[]
if VERSION > v"1.11-"
omus marked this conversation as resolved.
Show resolved Hide resolved
const PATCH_ENV = ScopedValue(PatchEnv())
with_active_env(body::Function, pe::PatchEnv) = with(body, PATCH_ENV => pe)
oxinabox marked this conversation as resolved.
Show resolved Hide resolved
else
function with_active_env(body::Function, pe::PatchEnv)
old_pe = get_active_env
try
PATCH_ENV[] = pe
body()
finally
PATCH_ENV[] = old_pe
end
end
omus marked this conversation as resolved.
Show resolved Hide resolved
end
get_active_env() = PATCH_ENV[]
omus marked this conversation as resolved.
Show resolved Hide resolved
omus marked this conversation as resolved.
Show resolved Hide resolved
4 changes: 2 additions & 2 deletions test/async.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
@test (@mock f()) == "mocked"

notify(c)
@test_broken take!(ch) == "original"
@test take!(ch) == "original"
omus marked this conversation as resolved.
Show resolved Hide resolved

# Task started inside patched context should call patched functions.
@async background()
Expand All @@ -35,6 +35,6 @@
end

notify(c)
@test_broken take!(ch) == "mocked"
@test take!(ch) == "mocked"
end
end
18 changes: 9 additions & 9 deletions test/concept.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,19 +20,19 @@
for p in patches
Mocking.apply!(pe, p)
end
Mocking.set_active_env(pe)

@test (@mock multiply(2)) == 8 # calls mocked `multiply(::Int)`
@test (@mock multiply(0x2)) == 0x6 # calls mocked `multiply(::Integer)`
@test (@mock multiply(2//1)) == 4//1 # calls original `multiply(::Number)`
Mocking.with_active_env(pe) do
@test (@mock multiply(2)) == 8 # calls mocked `multiply(::Int)`
@test (@mock multiply(0x2)) == 0x6 # calls mocked `multiply(::Integer)`
@test (@mock multiply(2//1)) == 4//1 # calls original `multiply(::Number)`

@test (@mock multiply(2)) != multiply(2)
@test (@mock multiply(0x2)) != multiply(0x2)
@test (@mock multiply(2//1)) == multiply(2//1)
@test (@mock multiply(2)) != multiply(2)
@test (@mock multiply(0x2)) != multiply(0x2)
@test (@mock multiply(2//1)) == multiply(2//1)
end

# Clean env
omus marked this conversation as resolved.
Show resolved Hide resolved
pe = Mocking.PatchEnv()
Mocking.set_active_env(pe)
@test Mocking.get_active_env() == Mocking.PatchEnv()

# Ensure that original behaviour is restored
@test (@mock multiply(2)) == 3
Expand Down
Loading