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

Testing: Deep equality of sets always passes #349

Closed
johnsonjo4531 opened this issue Apr 20, 2019 · 1 comment · Fixed by #350
Closed

Testing: Deep equality of sets always passes #349

johnsonjo4531 opened this issue Apr 20, 2019 · 1 comment · Fixed by #350

Comments

@johnsonjo4531
Copy link

johnsonjo4531 commented Apr 20, 2019

I would expect Sets to have different behavior than objects in deep equality testing. Currently all Set objects are evaulated as equal using std/testing/asserts.ts's equal method. As side notes I would expect new Set([1,2,3]) to equal new Set([3,2,1]) as order shouldn't matter, and I would also expect new Set([1,2,3]) to not equal a set with different length of elements such as new Set([1]) and I would also expect new Set([1,2,3]) to not equal a set with different elements such as new Set([1,2,4]).

Here is a gist that shows a failing test of the implementation and a passing test that uses my own equality method, equalsIncludingSet, that handles sets, but there is a problem in the equals method I wrote. The .has method of set does not check with deep equality therefore with the below code we are only shallowly comparing objects within the set, but I think atleast for now it may be better than saying all sets are equal. Due to random ordering still being equal in sets deep equality checking would seem to me to have atleast a O(n^2) check time (run through the set for each item and run through the other set for a deep equality match of that item) I mean we could narrow n down to only the objects within the set. Also showing differences between not equal sets would be another hurdle to jump that's not covered in my little example implementation.

Currently what fails in the test is this method assertNotEquals(setA, setB); in the withNormalAssertions test this is because as stated before all sets are currently being evaluated as equal even when they are not.

You can run the gist by running it's raw file like so:

deno https://gist.githubusercontent.com/johnsonjo4531/5f7ef8ce7c7420d67a0b6b43d979bd80/raw/57257491e5c900c6d4028f3245ba6d5d6859ae19/assertSetsTest.ts

Gist pasted below for reference.

import { runTests, test } from "https://deno.land/std@v0.3.4/testing/mod.ts";
import {
	assertNotEquals,
	assertEquals,
	equal
} from "https://deno.land/std@v0.3.4/testing/asserts.ts";

/**
 * determines if the two sets are equal order shouldn't matter
 */
function equalsIncludingSet(setA, setB) {
	if (setA instanceof Set || setB instanceof Set) {
		if (!(setA instanceof Set && setB instanceof Set)) {
			return false;
		}
		if (setA.size !== setB.size) {
			return false;
		}
		for (const item of setA) {
			if (!setB.has(item)) {
				return false;
			}
		}
		return true;
	}
	return equal(setA, setB);
}

function assertEqualsSet(setA, setB) {
	if (!equalsIncludingSet(setA, setB)) {
		throw new Error("SET A NOT EQUAL TO SET B");
	}
}

function assertNotEqualsSet(setA, setB) {
	if (equalsIncludingSet(setA, setB)) {
		throw new Error("SET A IS EQUAL TO SET B");
	}
}

test(function withNormalAssertions() {
	const setA = new Set([1, 2, 3]);
	const setB = new Set([1]);
	const setC = new Set([3, 2, 1]);
	assertEquals(setA, setC);
	assertNotEquals(setA, setB);
});

test(function withModifiedAssertionsForSets() {
	const setA = new Set([1, 2, 3]);
	const setB = new Set([1]);
	const setC = new Set([3, 2, 1]);
	assertEqualsSet(setA, setC);
	assertNotEqualsSet(setA, setB);
});

runTests();
@dsseng
Copy link
Contributor

dsseng commented Apr 20, 2019

@johnsonjo4531 I can work on this. Thanks for code

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 a pull request may close this issue.

2 participants