@types/qunit use same interface for skip, only, todo as for test.skip… by vstefanovic97 · Pull Request #70977 · DefinitelyTyped/DefinitelyTyped

vstefanovic97 · 2024-10-22T08:33:44Z

…, test.only, test.todo

Please fill in this template.

[+] Use a meaningful title for the pull request. Include the name of the package modified.
[+] Test the change in your own code. (Compile and run.)
[+] Add or edit tests to reflect the change.
[+] Follow the advice from the readme.
[+] Avoid common mistakes.
[+] Run pnpm test <package to test>.

Select one of these and delete the others:

If changing an existing definition:

[+] Provide a URL to documentation or source code which provides context for the suggested changes: <>
[+] If this PR brings the type definitions up to date with a new version of the JS library, update the version number in the package.json.

Since the previous PR actually broke overrides inside of ember-qunit the next steps are to fix the overrides over there.

First step is merging this PR

Second step in ember-qunit/types/index.d.ts we can just do

 namespace QUnit {
    interface TestFunction {
      <TC extends TestContext>(
        name: string,
        callback: (this: TC, assert: Assert) => void | Promise<unknown>
      ): void;
    }

    interface SkipFunction {
      <TC extends TestContext>(
        name: string,
        callback?: (this: TC, assert: Assert) => void | Promise<unknown>
      ): void;
    }

    interface TodoFunction {
      <TC extends TestContext>(
        name: string,
        callback: (this: TC, assert: Assert) => void | Promise<unknown>
      ): void;
    }

    interface OnlyFunction {
      <TC extends TestContext>(
        name: string,
        callback: (this: TC, assert: Assert) => void | Promise<unknown>
      ): void;
    }

Which will extend the overloads properly again

…, test.only, test.todo

typescript-bot · 2024-10-22T08:34:21Z

@vstefanovic97 Thank you for submitting this PR!

This is a live comment that I will keep updated.

1 package in this PR

qunit — on npm, on unpkg

Code Reviews

This PR can be merged once it's reviewed by a DT maintainer.

You can test the changes of this PR in the Playground.

Status

✅ No merge conflicts
✅ Continuous integration tests have passed
🕐 Only a DT maintainer can approve changes without tests

Once every item on this list is checked, I'll ask you for permission to merge and publish the changes.

Diagnostic Information: What the bot saw about this PR

{
  "type": "info",
  "now": "-",
  "pr_number": 70977,
  "author": "vstefanovic97",
  "headCommitOid": "03844370e59a18de8a4bda51671c1751f4078de9",
  "mergeBaseOid": "ce42f698cfa545277de0abe6bfc48974d64634f9",
  "lastPushDate": "2024-10-22T08:33:44.000Z",
  "lastActivityDate": "2024-10-29T18:49:39.000Z",
  "hasMergeConflict": false,
  "isFirstContribution": false,
  "tooManyFiles": false,
  "hugeChange": false,
  "popularityLevel": "Popular",
  "pkgInfo": [
    {
      "name": "qunit",
      "kind": "edit",
      "files": [
        {
          "path": "types/qunit/index.d.ts",
          "kind": "definition"
        }
      ],
      "owners": [
        "waratuman",
        "sechel",
        "gitKrystan",
        "jamescdavis",
        "wagenet",
        "Krinkle"
      ],
      "addedOwners": [],
      "deletedOwners": [],
      "popularityLevel": "Popular"
    }
  ],
  "reviews": [
    {
      "type": "changereq",
      "reviewer": "Krinkle",
      "date": "2024-10-29T18:49:39.000Z"
    },
    {
      "type": "approved",
      "reviewer": "RyanCavanaugh",
      "date": "2024-10-29T16:29:17.000Z",
      "isMaintainer": true
    }
  ],
  "mainBotCommentID": 2428623189,
  "ciResult": "pass"
}

typescript-bot · 2024-10-22T08:34:24Z

🔔 @waratuman @sechel @gitKrystan @jamescdavis @wagenet @Krinkle — please review this PR in the next few days. Be sure to explicitly select Approve or Request Changes in the GitHub UI so I know what's going on.

Krinkle · 2024-10-22T15:18:56Z

Thank you @vstefanovic97!

Krinkle · 2024-10-22T15:25:19Z

Does this mean QUnit.todo.each would become accepted by TS and suggested in IDEs? This is not documented or supported upstream. The QUnit.todo and other aliases are only for the individual function, not the other methods on its object.

I'm open to revisiting that but that's the current state. I do note these have doc comments above them which might narrow this to the function only?

https://qunitjs.com/api/QUnit/test.each/

vstefanovic97 · 2024-10-23T12:13:34Z

@Krinkle if I understend what you are saying

QUnit.test.todo.each

Is ok, but

QUnit.todo.each

Is not ok?

I agree that QUnit.todo.each is not documented, but I tried it and it seems to be implemented and working, same for skip and the other aliases.

See codepen example:

Am I missing something?

typescript-bot · 2024-10-26T12:56:28Z

⏳ Hi @vstefanovic97,

It's been a few days since this PR was approved by Krinkle and we're waiting for a DT maintainer to give a review.

If you would like to short-circuit this wait, you can edit some of the test files in the package that verify how the .d.ts files work. This would allow the PR to be merged by you or the DT module owners after a re-review.

Krinkle · 2024-10-26T22:51:14Z

@vstefanovic97 I understand that it technically works, but it's not supported. It works because we re-use the same function by reference. There hasn't yet been an internal reason to wilfully break it, although now that we know about it, maybe we will do that in QUnit 3.

The top-level aliases predate the existence of each(). There is no reason for anyone to use "each" via that route (I'm open to use cases), and no reason to my knowledge to encourage a bifurcation in the ecosystem by having two ways to do the same thing. If it ends up suggested in IDEs it seems likely it'll be used by someone who would've been equally happy to use it the normal way. Except then there'd be an inconsistency. This in turn makes code review, static analysis, and ESLint rules harder to get right.

vstefanovic97 · 2024-10-28T09:48:27Z

@Krinkle well in that case I guess we shouldn't even merge this then?
And I'll just make a PR to ember-qunit to fix the method overrides for what is already here on master.

Krinkle · 2024-10-28T13:36:54Z

@vstefanovic97 Yeah, although the optional callback fix would be welcome. And, if it helps with ember-qunit I would also welcome any reuse like TestFunctionCallback.

It may also be possible to structure it such that SkipFunction etc contain only the function and add the "each" inline by union, or separately; if that helps!

Krinkle

Per comment thread.

typescript-bot · 2024-10-29T18:50:19Z

@vstefanovic97 One or more reviewers has requested changes. Please address their comments. I'll be back once they sign off or you've pushed new commits. Thank you!

vstefanovic97 · 2024-10-30T08:28:06Z

@Krinkle as per agreement I'm closing this and will open a new PR for just this minor fixes, refactors

vstefanovic97 · 2024-10-30T08:43:11Z

@Krinkle I have opened #71036 to fix the optional callback part, after that I'll fix the overloads in ember-qunit

@types/qunit use same interface for skip, only, todo as for test.skip…

0384437

…, test.only, test.todo

typescript-bot added Popular package This PR affects a popular package (as counted by NPM download counts). Untested Change This PR does not touch tests labels Oct 22, 2024

vstefanovic97 mentioned this pull request Oct 22, 2024

@types/qunit - Add types for test.each for qunit #70946

Merged

Krinkle approved these changes Oct 22, 2024

View reviewed changes

typescript-bot added the Owner Approved A listed owner of this package signed off on the pull request. label Oct 22, 2024

RyanCavanaugh approved these changes Oct 29, 2024

View reviewed changes

typescript-bot added Maintainer Approved Self Merge This PR can now be self-merged by the PR author or an owner labels Oct 29, 2024

Krinkle suggested changes Oct 29, 2024

View reviewed changes

typescript-bot added Revision needed This PR needs code changes before it can be merged. and removed Self Merge This PR can now be self-merged by the PR author or an owner Owner Approved A listed owner of this package signed off on the pull request. Maintainer Approved labels Oct 29, 2024

vstefanovic97 closed this Oct 30, 2024

vstefanovic97 mentioned this pull request Oct 30, 2024

[@types/qunit] fix optional callback for test.todo and test.skip #71036

Merged

Conversation

vstefanovic97 commented Oct 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

typescript-bot commented Oct 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

1 package in this PR

Code Reviews

Status

Uh oh!

typescript-bot commented Oct 22, 2024

Uh oh!

Krinkle commented Oct 22, 2024

Uh oh!

Krinkle commented Oct 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vstefanovic97 commented Oct 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

typescript-bot commented Oct 26, 2024

Uh oh!

Krinkle commented Oct 26, 2024

Uh oh!

vstefanovic97 commented Oct 28, 2024

Uh oh!

Krinkle commented Oct 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Krinkle left a comment

Choose a reason for hiding this comment

Uh oh!

typescript-bot commented Oct 29, 2024

Uh oh!

vstefanovic97 commented Oct 30, 2024

Uh oh!

vstefanovic97 commented Oct 30, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

vstefanovic97 commented Oct 22, 2024 •

edited

Loading

typescript-bot commented Oct 22, 2024 •

edited

Loading

Krinkle commented Oct 22, 2024 •

edited

Loading

vstefanovic97 commented Oct 23, 2024 •

edited

Loading

Krinkle commented Oct 28, 2024 •

edited

Loading