nodejs-polars icon indicating copy to clipboard operation
nodejs-polars copied to clipboard

fix: Do not use `constructor.name` to check object

Open roll opened this issue 1 month ago • 3 comments

Other projects running into the same problem:

  • https://github.com/node-fetch/node-fetch/issues/667

  • [x] fixes #372
  • [x] adds type checking to yarn precommit
  • [ ] fix failing tests

Not sure what is correct behaviour for these tests and why they fail now e.g.:

  ● series functions › 3 dtype: expected matches actual

    expect(received).toEqual(expected) // deep equality

    - Expected  - 1
    + Received  + 1

      Object {
    -   "DataType": "Utf8",
    +   "DataType": "String",
      }

One test failing locally that is not related to the PR:

 FAIL  __tests__/dataframe.test.ts (6.276 s)
  ● additional › upsample


    Expected:
    >>shape: (7, 3)
    ┌────────────┬────────┬────────┐
    │ date       ┆ groups ┆ values │
    │ ---        ┆ ---    ┆ ---    │
    │ date       ┆ str    ┆ f64    │
    ╞════════════╪════════╪════════╡
    │ 2024-02-01 ┆ A      ┆ 0.0    │
    │ 2024-03-01 ┆ A      ┆ 0.0    │
    │ 2024-03-31 ┆ A      ┆ 0.0    │
    │ 2024-04-30 ┆ A      ┆ 2.0    │
    │ 2024-03-31 ┆ B      ┆ 1.0    │
    │ 2024-04-30 ┆ B      ┆ 1.0    │
    │ 2024-05-31 ┆ B      ┆ 3.0    │
    └────────────┴────────┴────────┘
    Received:
    >>shape: (6, 3)
    ┌────────────┬────────┬────────┐
    │ date       ┆ groups ┆ values │
    │ ---        ┆ ---    ┆ ---    │
    │ date       ┆ str    ┆ f64    │
    ╞════════════╪════════╪════════╡
    │ 2024-02-01 ┆ A      ┆ 0.0    │
    │ 2024-03-01 ┆ A      ┆ 0.0    │
    │ 2024-04-01 ┆ A      ┆ 0.0    │
    │ 2024-03-31 ┆ B      ┆ 1.0    │
    │ 2024-04-30 ┆ B      ┆ 1.0    │
    │ 2024-05-31 ┆ B      ┆ 3.0    │
    └────────────┴────────┴────────┘

roll avatar Oct 25 '25 10:10 roll

Python Polars returns String for pl.Series(["foo"]).dtype

Bidek56 avatar Oct 25 '25 20:10 Bidek56

bun run test --findRelatedTests __tests__/dataframe.test.ts are passing on my laptop on this branch.

Bidek56 avatar Oct 25 '25 20:10 Bidek56

@roll fix or nix? Thx

Bidek56 avatar Nov 03 '25 22:11 Bidek56

@Bidek56 Please take a look:

  • I had to skip the upsample test as it fails for me both on main/pr (not sure how CI passes on Github)
  • I change Utf8 to String in a few comparison tests as it seems a correct behaviour as per https://github.com/pola-rs/nodejs-polars/pull/385#issuecomment-3447750958

roll avatar Nov 08 '25 14:11 roll

@Bidek56 Please take a look:

LGTM, Can you please not skip skip the upsample test? It passed for me locally and on CI. Thx

Bidek56 avatar Nov 08 '25 15:11 Bidek56

Thanks @Bidek56, thanks for looking into it!

I enabled test I think I confused it last time it seems actually passed on CI initially

roll avatar Nov 08 '25 16:11 roll