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

Context proxying doesn't work with Date #39

Closed
matthewp opened this issue Aug 2, 2020 · 10 comments
Closed

Context proxying doesn't work with Date #39

matthewp opened this issue Aug 2, 2020 · 10 comments
Labels
bug Something isn't working

Comments

@matthewp
Copy link

matthewp commented Aug 2, 2020

Not sure the reason, stepped through the code briefly. Something like this doesn't work:

test('whatever', context => {
  let { date } = context;

  date.getTime(); // throws.
});

I think getTime() doesn't like it when the receiver is a proxy.

@lukeed
Copy link
Owner

lukeed commented Aug 8, 2020

Thanks for this – it's been interesting to play with... 😅

Date wants to be bound. If I bind it to itself (either through the obj or 3rd receiver param), the Date error will go away, but it also allows things like context.date.setHours to mutate the value successfully.

Additionally, setting up that binding will make things like context.array.push(1, 2, 3) mutate the array. Right now, it pretends to by returning the new length, but the backing value doesn't change.

I'm still playing with it. Don't want to go down the detect-every-constructor route, because that list will be never ending, especially with custom classes. Looking at Object.freeze next

@lukeed lukeed added the bug Something isn't working label Aug 8, 2020
@101arrowz
Copy link

This just bit me too; I tried using Typed Arrays in context. Can't use Buffer.from, slice, subarray, and a lot of other non-mutating methods.

My question is why do you need to prevent mutations in the context? Performance and compatibility dramatically increase by not using Proxy, plus you wouldn't have to deal with the headache of ensuring that the mutation prevention system works for every custom class. It's not like it's impossible to just use a variable outside of the context to store state, if someone REALLY wanted to mutate in the tests. To me, it doesn't seem possible to uphold total semantic correctness, so why do any at all?

@lukeed
Copy link
Owner

lukeed commented Sep 24, 2020

Yes, this is a possibility I'm considering. The initial idea was that preventing context mutation would unlock the ability for a suite's tests to be run in parallel (when the feature came).

@matthewp
Copy link
Author

You could have the context object in the before() and just warn people that it's mutable, then in before.each have a separate context object that is Object.create(beforeContext) and that one will be scoped to each test, so immutability is preserved aside from changes to the before context.

@lukeed
Copy link
Owner

lukeed commented Sep 26, 2020

Unfortunately that wouldn't be immutable – and from a performance standpoint, it'd be really expensive to Object.create per test. Quick example of the immutability issue (or, how I understand your idea at least):

// suite.before
let context = { foo: [1, 2, 3] };
// suite.before.each
let copy = Object.create(context);
// my test
copy.foo.push(1, 2, 3);

console.log(context.foo);
//=> [1, 2, 3, 1, 2, 3]

@matthewp
Copy link
Author

Yeah, I meant that properties you add to the context in the before.each are immutable. If you mutate the proto you are mutating the before context and that is of course shared. In my experience most before type things you want to do can be done in a before.each and that should be the preferred place to do them.

@dimfeld
Copy link

dimfeld commented Oct 12, 2020

Just to add in, Set.values and Map.entries and similar functions have the same issue. Quick test case:

import { test } from 'uvu';
test.before((context) => {
  context.map = new Map();
  context.set = new Set();
});

test('map.entries()', ({ map }) => {
  map.entries();
});

test('set.values()', ({ set }) => {
  set.values();
});

@lukeed
Copy link
Owner

lukeed commented Oct 13, 2020

Ya I'll probably have to drop the immutability locker. Was a nice idea but doesn't seem to be all that viable (sans cloning objects before each test).

If you consider this a must-have, please say so now before it's too late 😉

@matthewp
Copy link
Author

@lukeed It's cool if you don't like the Object.create() idea but just to be clear it's not a copy or a clone. Creating an object is not O(n), it's equivalent to the cost of creating a new object in a function which is something people do constantly. I'd be very surprised if you could create a benchmark, even a really unrealistic one, that could show a perf cost to this. This is just testContext = { __proto__: suiteContext } and that's all.

Again, happy with whatever decision you prefer, just felt the need to defend prototypes a bit 😀

@lukeed
Copy link
Owner

lukeed commented Oct 13, 2020

@matthewp I wasn't talking about Object.create in my last comment :) Yes it's a new object, but as demonstrated in my snippet above, Object.create(context) doesn't work because (as you've pointed out) the context is mounted as a prototype. Any changes (to existing keys) on the Object.create would affect the original context, at which point there's no immutability protection. That kinda defeats the purpose IMO since the point of context is to pass around shared values – if those are still vulnerable – and only new keys got washed away after each test – then there's no real point in Object.createing at all. I would be just as well off using local variables inside my test.

Creating a new object and then assigning the keys to that object would then be a copy/clone, and that's where the performance question comes in. This is what klona is doing, and while it's currently the fastest of the deep-cloners, including it in a place like before.each would still have some measurable impact on test runtime. Here's a hodgepodge of a few different metrics -- also included plain object creators (pojo, nCreate, oCreate) just to illustrate the cost between them.

While it may be obvious, an important, unrelated callout is that if you ever see Object.create({}), replace that with {} or Object.create(Object.prototype) for free performance boost. We're talking 2B vs 2M ops/sec~!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants