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

Allow spy.on with tables that error on __newindex #174

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

S-S-X
Copy link

@S-S-X S-S-X commented Mar 14, 2021

Allows using spy with tables that error on __newindex.

For example:

assert = require("luassert")
spy = require("luassert.spy")

local MyCls = {}
function MyCls:foo() end
function MyCls:__newindex() error("No new indexes for you!") end

myinstance = setmetatable({}, MyCls)

spy.on(myinstance, "foo")
myinstance:foo()
assert.spy(myinstance.foo).was.called(1)
print("Success\n")

Would fail on spy.on(myinstance, "foo") because of error No new indexes for you!.
With rawset it succeeds because __newindex is never called.

Executing test script

Example output without changes from this PR:

lua: test.lua:6: No new indexes for you!
stack traceback:
	[C]: in function 'error'
	test.lua:6: in function <test.lua:6>
	.../.luarocks/share/lua/5.1/luassert/spy.lua:93: in function 'on'
	test.lua:10: in main chunk
	[C]: ?

With changes from this PR:

Success

What and why it should use rawset

This might affect some use cases where it is expected that __newindex is called, any ideas and is this acceptable approach?
However I think adding spy should not be visible to target or call any metamethods of target, taking this into account I think this would be most correct quick and easy fix.

@Tieske
Copy link
Member

Tieske commented Mar 15, 2021

Good catch. Would you mind adding a test? (possibly also one for a stub if that makes sense)

@Tieske
Copy link
Member

Tieske commented Mar 21, 2021

I'm having second thoughts about this. This might have side effects, the table is protected for a reason, just overriding that behaviour is dangerous.

I think possible side-effects for this aren't worth the fix for this particular case. Maybe we should skip this one...

wdyt @DorianGray ?

@S-S-X
Copy link
Author

S-S-X commented Aug 29, 2021

I'm having second thoughts about this. This might have side effects, the table is protected for a reason, just overriding that behaviour is dangerous.

You're kind of right but thing is original behavior is also dangerous because normally you wont expect meta methods to be called when you're hooking debugger into your code.

For debugger (like what spy basically is) all possible side effects should be avoided, yes it would be nice if one could track calls and modifications without altering original object but that would be way more complicated change.

For simple example __newindex could be used to implement entry counter for you class, if debugger causes counter incrementing then your program behavior will be different with debugger. This has nasty side effects like off by one errors passing tests when spy is used but program actually failing when you're actually running it without tests.

So basically why I think it would be more correct to use rawset is simply that tests should not alter program behavior if it is anyhow possible to avoid altering behavior.

@S-S-X
Copy link
Author

S-S-X commented Aug 29, 2021

But then again for same reason explained in previous message:

This might affect some use cases where it is expected that __newindex is called

With that I basically mean that some might have written tests and tested program in a way that program is actually aware of tests and expects tests to alter behavior, while this is wrong approach it still should be take into account because altering that behavior can cause some tests that succeed before to fail afterwards.

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

Successfully merging this pull request may close these issues.

2 participants