fix(testing/equals): cater for different class constructor functions #1000

gtramontina · 2021-07-03T04:17:06Z

While writing a polymorphic Either, I stumbled upon this weird comparison behavior: I have two classes that are structurally equal class Left<L> { private value: L } and class Right<R> { private value: R }. When comparing them with assertEquals from https://deno.land/[email protected]/testing/asserts.ts, I've received what feels like a false positive.

Example

class A {
  private val = 1;
}
class B {
  private val = 1;
}

assertEquals(new A(), new B()); // I'd expect this to fail, but it passes

I'm not sure if this is the intended original behavior, but it sure is unexpected. This fix compares whether the object constructors are the same at the very end of the equals function.

CLAassistant · 2021-07-03T04:17:10Z

All committers have signed the CLA.

gtramontina · 2021-07-03T05:57:52Z

By the way, I forgot to mention that I couldn't run the full suite of tests locally, so I was hoping the the pipeline would catch some more failures, which it did.

From what I could tell, it seems that the majority are indeed type differences. Some examples:

-   Uint8Array(4) [
+   [

-   Buffer(4) [
+   Uint8Array(4) [

To me they look like legit/expected failures given the type difference. Keen to hear your thoughts. 💭

wperron · 2021-07-04T15:37:18Z

If you're having issues running the test suite, don't hesitate to reach out, either on the Discord server or to someone directly (my email is on my github profile, my inbox is always open)

As for the fix itself, I'm not sure I entirely agree that the current behavior is unexpected; JavaScript's type system is based on duck typing, so if an object has property foo and has function bar() then it's probably the type Foobar. It's a subtlety of JavaScript, but the current implementation of assertEquals doesn't prevent checking for assertEquals(a.constructor, b.constructor)

It'd be interesting to see how other existing JavaScript test frameworks handle this before going forward with this fix.

gtramontina · 2021-07-05T00:22:27Z

Thanks for making yourself available! I've managed to go further with the tests, though. For context, I'm running like the workflow describes, in a docker container – if I run into more issues I'll definitely reach out.
https://github.com/denoland/deno_std/blob/1914407fc82311efd04ab1fc702c239cf6ac70fd/.github/workflows/ci.yml#L32-L33

In relation to other JS test libs, Node's own assert (both .equal and .strictEqual) behave the way I described. So does Chai.

Drawing from experience in other dynamically typed languages such as Ruby and Python where duck-typing is prevalent, equality assertions always fail on different types.

(opinion notice, take it with a grain of salt 😄) I wouldn't rely on duck typing in tests especially, where we want to ascertain behavior, and being of a certain type conveys that. I wouldn't want to work with "probably" in my tests; I would want to be sure. (please note that I put the word "probably" in quotes to emphasize it, not to mock what you said).

bartlomieju · 2021-07-06T15:25:42Z

Thanks for opening the PR @gtramontina! I agree with you that instances of different classes shouldn't be equal to each other - even if they have effectively the same interface (or shape). I think we should go forward with your PR and it's good that it actually caught these discrepancies. Would you mind fixing currently failing tests?

gtramontina · 2021-07-09T03:39:37Z

I'll fix the failing tests when I get some down time and we can take it from there. 👍 Cheers!

kitsonk · 2021-07-17T10:36:51Z

testing/asserts.ts

@@ -202,6 +202,9 @@ export function equal(c: unknown, d: unknown): boolean {
        if (!(a instanceof WeakRef && b instanceof WeakRef)) return false;
        return compare(a.deref(), b.deref());
      }
+      if (a && b && (a.constructor !== b.constructor)) {


Feels like this should go a lot earlier in the check, as it would short circuit much quicker.

I agree. I tried adding it closer to top, but wasn't being too successful (was getting unexpected failures). So I put it at the end as it was the least disruptive place I could find and make the new test pass. This way I could at least get something out and get feedback. Once I get the rest of the tests fixed, I'll try moving it earlier.

caspervonb · 2021-07-17T10:58:14Z

Won't this break a lot of loose comparisons? feels like goes against duck typing too far into strict typing.

gtramontina · 2021-07-17T13:21:52Z

Won't this break a lot of loose comparisons? feels like goes against duck typing too far into strict typing.

It didn't break that many… Majority of them were related to buffers and arrays (vide 09484d4).

Regarding duck typing, I've presented my opinion earlier (#1000 (comment)). Happy to chat more about it…

Edit: Ah! You probably meant breaking a lot of comparisons on user-land. Indeed! Good call! If we're moving forward with this, it definitely needs to be tagged as a breaking change!

gtramontina · 2021-07-17T13:31:41Z

Regarding the two remaining failures:

The first one seems to be a real issue with _util/deep_assign.ts revealed by comparing types. The failure presents the following diff:

-     reg: {},
+     reg: /DENOWOWO/,

After a preliminary test, it seems that deep_assign might have problems with copying prototypes or something along these lines. I'll see if I can dig a bit deeper here.

The second failure is on a test that seems to be asserting on an internal state; potentially on a structure that was meant to remain unexported?

-   TarEntry {
+   {

Any thoughts on these? 💭

bartlomieju · 2021-07-21T11:12:20Z

Regarding the two remaining failures:

The first one seems to be a real issue with _util/deep_assign.ts revealed by comparing types. The failure presents the following diff:
-     reg: {},
+     reg: /DENOWOWO/,
After a preliminary test, it seems that deep_assign might have problems with copying prototypes or something along these lines. I'll see if I can dig a bit deeper here.

The second failure is on a test that seems to be asserting on an internal state; potentially on a structure that was meant to remain unexported?
-   TarEntry {
+   {
Any thoughts on these? 💭

@gtramontina thanks for digging into that, indeed the first failure seems like an actual problem with deep_assign.ts that needs to be addressed. As for the second failure, I'm fine with changing the test.

gtramontina · 2021-07-21T11:14:39Z

I've literally just pushed some updates… 😅 I'll add some thoughts as inline comments. I've also adjusted the commit message so it is tagged as a breaking change as pointed out by @caspervonb.

gtramontina · 2021-07-21T11:18:54Z

_util/deep_assign.ts

+      if (value instanceof RegExp) {
+        target[key] = new RegExp(value);
+        return;
+      }


I'm not quite happy with the fix as it seems deep_assign is still flawed. I'm not sure about its usecases, given it lives in a _util directory… Looks like it is supposed to be something private?

Anyway, as fixing this implementation in particular is not the goal of this PR, I guess we could come back to it later.

gtramontina · 2021-07-21T11:20:59Z

archive/tar_test.ts

@@ -397,7 +397,7 @@ Deno.test("untarLinuxGeneratedTar", async function () {
    const content = expected.content;
    delete expected.content;

-    assertEquals(entry, expected);
+    assertEquals({ ...entry }, expected);


This seems a bit hacky, but given that TarEntry is a private type, downcasting it to object in order to keep the assertion behaving as before seems to be an ok trade-off (?).

…ions This commit also fixes the tests that required adjustments. BREAKING CHANGE: `assertEquals`, from `testing/assert.ts`, now compares class constructors. Meaning that if two objects are structurally identical but are of different types, the assertion will now fail.

kt3k

@gtramontina Nice work! LGTM

kitsonk reviewed Jul 17, 2021

View reviewed changes

bartlomieju mentioned this pull request Jul 19, 2021

asserts: False positives on Set/non-Set equality #1044

Closed

gtramontina commented Jul 21, 2021

View reviewed changes

bartlomieju requested a review from kt3k July 21, 2021 11:25

kt3k approved these changes Jul 21, 2021

View reviewed changes

kt3k merged commit c27b259 into denoland:main Jul 22, 2021

gtramontina deleted the fix-class-equals branch July 23, 2021 00:56

kt3k mentioned this pull request Sep 29, 2021

[std][testing] equal should check type #676

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(testing/equals): cater for different class constructor functions #1000

fix(testing/equals): cater for different class constructor functions #1000

gtramontina commented Jul 3, 2021

CLAassistant commented Jul 3, 2021 •

edited

Loading

gtramontina commented Jul 3, 2021 •

edited

Loading

wperron commented Jul 4, 2021

gtramontina commented Jul 5, 2021

bartlomieju commented Jul 6, 2021

gtramontina commented Jul 9, 2021

kitsonk Jul 17, 2021

gtramontina Jul 17, 2021

caspervonb commented Jul 17, 2021 •

edited

Loading

gtramontina commented Jul 17, 2021 •

edited

Loading

gtramontina commented Jul 17, 2021

bartlomieju commented Jul 21, 2021

gtramontina commented Jul 21, 2021

gtramontina Jul 21, 2021 •

edited

Loading

gtramontina Jul 21, 2021

kt3k left a comment

fix(testing/equals): cater for different class constructor functions #1000

fix(testing/equals): cater for different class constructor functions #1000

Conversation

gtramontina commented Jul 3, 2021

CLAassistant commented Jul 3, 2021 • edited Loading

gtramontina commented Jul 3, 2021 • edited Loading

wperron commented Jul 4, 2021

gtramontina commented Jul 5, 2021

bartlomieju commented Jul 6, 2021

gtramontina commented Jul 9, 2021

kitsonk Jul 17, 2021

Choose a reason for hiding this comment

gtramontina Jul 17, 2021

Choose a reason for hiding this comment

caspervonb commented Jul 17, 2021 • edited Loading

gtramontina commented Jul 17, 2021 • edited Loading

gtramontina commented Jul 17, 2021

bartlomieju commented Jul 21, 2021

gtramontina commented Jul 21, 2021

gtramontina Jul 21, 2021 • edited Loading

Choose a reason for hiding this comment

gtramontina Jul 21, 2021

Choose a reason for hiding this comment

kt3k left a comment

Choose a reason for hiding this comment

CLAassistant commented Jul 3, 2021 •

edited

Loading

gtramontina commented Jul 3, 2021 •

edited

Loading

caspervonb commented Jul 17, 2021 •

edited

Loading

gtramontina commented Jul 17, 2021 •

edited

Loading

gtramontina Jul 21, 2021 •

edited

Loading