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

Tests for Set methods #3738

Closed
18 tasks done
bakkot opened this issue Dec 1, 2022 · 7 comments · Fixed by #3966
Closed
18 tasks done

Tests for Set methods #3738

bakkot opened this issue Dec 1, 2022 · 7 comments · Fixed by #3966

Comments

@bakkot
Copy link
Contributor

bakkot commented Dec 1, 2022

The Set methods proposal is now stage 3 (🎉 ). That means it's time for tests! Here's the things which occur to me offhand; feel free to edit this issue to add more.

  • for all seven methods
    • basic interactions between native Sets
      • including extremal cases, i.e. when either the receiver or the argument is empty and when the receiver and the argument are the same value
    • works when the argument is a userland class which implements size/has/keys
      • calls the appropriate methods on the userland class in the appropriate order, which for some algorithms varies depending on the relative sizes of the receiver vs the argument
    • works when the argument is a Map (treating it a Set of its keys)
    • throws if the argument lacks size or has or keys
      • throws if the argument's size is, or coerces to, NaN
      • throws if the argument's has or keys is non-callable
      • as a special case, throws if passed an Array
    • works when the receiver is a Set subclass
      • but does not read the receiver's size, has, or keys properties, nor those of Set.prototype
    • throws when the receiver is not a Set
  • for each of union, intersection, difference, symmetricDifference
    • produces a native Set
      • even if the receiver is a subclass
      • even if Symbol.species is overridden; Symbol.species is not inspected at all)
    • does not invoke Set.prototype.add to construct the result (testing this requires mutating Set.prototype)
    • order of the result, both when the receiver is smaller and larger than the argument (which affects the algorithm used in some cases)
      • order of the result when the argument is a userland class which mutates the receiver during execution of the algorithm

This list previously included a special case for a sorting step in intersection, but that step was removed in tc39/proposal-set-methods#94. The new behavior should now be covered by the "order of the result, both when the receiver is smaller and larger than the argument" case above.

@frehner
Copy link
Contributor

frehner commented Apr 24, 2023

Hello @bakkot thanks for this issue! I'm a first time contributor to test262, in the process of writing some tests for these features (see #3816 ) and am also cross-referencing this todo list as I look to add more tests.

Forgive my ignorance, but I had a question about:

does not invoke Set.prototype.add to construct the result (testing this requires mutating Set.prototype)

Is that related to this section in the details.md doc? https://github.com/tc39/proposal-set-methods/blob/main/details.md#constructing-a-set-directly

Being very new to this process, I was wondering if something like that needs to be explicitly called out in the spec or if it's common to just keep it in something like details.md.

Thank you 🙂

@bakkot
Copy link
Contributor Author

bakkot commented Apr 24, 2023

Is that related to this section in the details.md doc? https://github.com/tc39/proposal-set-methods/blob/main/details.md#constructing-a-set-directly

Yup.

I was wondering if something like that needs to be explicitly called out in the spec or if it's common to just keep it in something like details.md

It doesn't need to be called out in the spec - it's something you can conclude already from looking at the algorithm and seeing that it does not invoke add or the Set constructor at any point.

I usually try to call these things out in a separate document, but not all proposals will have such a thing.

@frehner
Copy link
Contributor

frehner commented Apr 25, 2023

Thank you very much for the context!

@frehner
Copy link
Contributor

frehner commented May 22, 2023

#3816 implements the following for union:

  • for all seven methods

    • basic interactions between native Sets

      • including extremal cases, i.e. when either the receiver or the argument is empty and when the receiver and the argument are the same value
    • works when the argument is a userland class which implements size/has/keys

      • calls the appropriate methods on the userland class in the appropriate order, which for some algorithms varies depending on the relative sizes of the receiver vs the argument

I don't think this applies to union, but if it does, I could maybe use a little clarification on what this means.

  • works when the argument is a Map (treating it a Set of its keys)

  • throws if the argument lacks size or has or keys

    • throws if the argument's size is, or coerces to, NaN
    • throws if the argument's has or keys is non-callable
    • as a special case, throws if passed an Array
  • works when the receiver is a Set subclass

    • but does not read the receiver's size, has, or keys properties, nor those of Set.prototype
  • throws when the receiver is not a Set

I'm not entirely sure I understand this one.

  • for each of union, intersection, difference, symmetricDifference

    • produces a native Set

      • even if the receiver is a subclass
      • even if Symbol.species is overridden; Symbol.species is not inspected at all)
    • does not invoke Set.prototype.add to construct the result (testing this requires mutating Set.prototype)

    • order of the result, both when the receiver is smaller and larger than the argument (which affects the algorithm used in some cases)

      • order of the result when the argument is a userland class which mutates the receiver during execution of the algorithm

I don't think I fully understand this one, sorry.


Any insights on the above would be helpful. Thank you!

@bakkot
Copy link
Contributor Author

bakkot commented May 23, 2023

calls the appropriate methods on the userland class in the appropriate order, which for some algorithms varies depending on the relative sizes of the receiver vs the argument

I don't think this applies to union, but if it does, I could maybe use a little clarification on what this means.

The second half of the original bullet doesn't apply to union, but the first half does.

What this means is that if you pass something which is not a Set as an argument to .union (or other methods), methods on that thing will be looked up and invoked, and there are many points at which that is observable. For example,

function observableIterator() {
  let values = ['a', 'b'];
  let index = 0;
  return {
    get next() {
      console.log('getting next');
      return function () {
        console.log('calling next');
        return {
          get done() {
            console.log('getting done');
            return index >= values.length;
          },
          get value() {
            console.log('getting value');
            return values[index++];
          },
        };
      };
    },
  };
}

let observableSetLike = {
  get size() {
    console.log('getting size');
    return {
      valueOf() {
        console.log('coercing size');
        return 2;
      },
    };
  },
  get has() {
    console.log('getting has');
    return function (arg) {
      console.log('looking up', arg);
      return arg === 'a' || arg === 'b';
    };
  },
  get keys() {
    console.log('getting keys');
    return function () {
      console.log('calling keys');
      return observableIterator();
    };
  },
};

new Set().union(observableSetLike); // prints a whole bunch of stuff and ultimately ends up with a Set holding two elements 'a' and 'b'

throws when the receiver is not a Set

I'm not entirely sure I understand this one.

The "receiver" is the thing on which a method is invoked. So for example this method should throw when invoked with the observableSetLike I defined above as the receiver:

Set.prototype.union.call(observableSetLike, new Set); // throws, doesn't print anything

order of the result when the argument is a userland class which mutates the receiver during execution of the algorithm
I don't think I fully understand this one, sorry.

As above, you can have something like observableSetLike which invokes code during the execution of the algorithm. And that can actually mutate the receiver, which can affect the results. For example:

let baseSet = new Set(['a', 'b', 'c', 'd', 'e']);

function mutatingIterator() {
  let index = 0;
  let values = ['x', 'y'];
  return {
    next() {
      baseSet.delete('b');
      baseSet.delete('c');
      baseSet.add('b');
      baseSet.add('d');
      return {
        done: index >= 2,
        value: values[index++],
      };
    },
  };
}

let evilSetLike = {
  size: 2,
  has() {
    throw new Error('should not be invoked in this case example');
  },
  keys() {
    return mutatingIterator();
  },
};

console.log([...baseSet.union(evilSetLike)]); // ['a', 'b', 'c', 'd', 'e', 'x', 'y']
console.log([...baseSet]); // ['a', 'd', 'e', 'b']

The bit to note here is that baseSet was mutated by the call to union. We are checking the order of the results when that happens. In this case, because the original set data is cloned early on in the algorithm, the mutation should not affect the result.

@frehner
Copy link
Contributor

frehner commented Jun 3, 2023

@bakkot thank you very much for those examples! I added those tests in this commit and additionally added your name to the copyright as I mostly used your example code to write them. (Let me know if you want me to remove that attribution, though - I'll do what you feel is best).

I think that wraps it up for union, I hope!

@Ms2ger
Copy link
Contributor

Ms2ger commented Oct 26, 2023

Ref #3949

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

Successfully merging a pull request may close this issue.

4 participants