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

jasmine: update spec and suite #52004

Merged
merged 5 commits into from
May 11, 2021

Conversation

UziTech
Copy link
Contributor

@UziTech UziTech commented Mar 26, 2021

Please fill in this template.

If changing an existing definition:

@UziTech
Copy link
Contributor Author

UziTech commented Mar 26, 2021

TODO:

@typescript-bot
Copy link
Contributor

👋 Hi there! I’ve run some quick measurements against master and your PR. These metrics should help the humans reviewing this PR gauge whether it might negatively affect compile times or editor responsiveness for users who install these typings.

Let’s review the numbers, shall we?

Comparison details 📊
master #52004 diff
Batch compilation
Memory usage (MiB) 91.5 91.8 +0.3%
Type count 15062 15050 0%
Assignability cache size 5473 5473 0%
Language service
Samples taken 2887 2887 0%
Identifiers in tests 2887 2887 0%
getCompletionsAtPosition
    Mean duration (ms) 370.4 372.6 +0.6%
    Mean CV 8.6% 8.3%
    Worst duration (ms) 592.9 536.7 -9.5%
    Worst identifier onload callThrough
getQuickInfoAtPosition
    Mean duration (ms) 371.1 373.9 +0.8%
    Mean CV 8.9% 8.5%
    Worst duration (ms) 549.5 566.1 +3.0%
    Worst identifier and toHaveBeenCalledWith

It looks like nothing changed too much. I won’t post performance data again unless it gets worse.

@typescript-bot typescript-bot added the Perf: Same typescript-bot determined that this PR will not significantly impact compilation performance. label Mar 26, 2021
types/jasmine/index.d.ts Outdated Show resolved Hide resolved
types/jasmine/index.d.ts Outdated Show resolved Hide resolved
@typescript-bot
Copy link
Contributor

Updated numbers for you here from d5189d1.

These typings are for a version of jasmine that doesn’t yet exist on master, so I’ve compared them with v3.6.

Comparison details 📊
3.6@master 3.7 in #52004 diff
Batch compilation
Memory usage (MiB) 90.2 94.6 +5.0%
Type count 15186 15653 +3%
Assignability cache size 5418 5582 +3%
Language service
Samples taken 2887 3291 +14%
Identifiers in tests 2887 3291 +14%
getCompletionsAtPosition
    Mean duration (ms) 446.4 368.8 -17.4%
    Mean CV 6.8% 8.0%
    Worst duration (ms) 721.6 526.0 -27.1% 🌟
    Worst identifier badness MAX_PRETTY_PRINT_DEPTH
getQuickInfoAtPosition
    Mean duration (ms) 449.7 369.3 -17.9%
    Mean CV 7.1% 8.0%
    Worst duration (ms) 635.3 573.2 -9.8%
    Worst identifier objectContaining it
System information
Node version v14.16.0 v14.16.1
CPU count 2 2
CPU speed 2.294 GHz 2.095 GHz
CPU model Intel(R) Xeon(R) CPU E5-2673 v4 @ 2.30GHz Intel(R) Xeon(R) Platinum 8171M CPU @ 2.60GHz
CPU Architecture x64 x64
Memory 6.8 GiB 6.8 GiB
Platform linux linux
Release 4.15.0-1111-azure 4.15.0-1113-azure

First off, note that the system varied slightly between these two runs, so you’ll have to take these measurements with a grain of salt.

Wow, it looks like all the big movers moved in the right direction! Way to go! 🌟 I won’t post performance data again unless it gets worse.

@typescript-bot typescript-bot added Perf: Better typescript-bot determined that this PR improves compilation performance. and removed Perf: Same typescript-bot determined that this PR will not significantly impact compilation performance. labels May 8, 2021
@UziTech UziTech marked this pull request as ready for review May 10, 2021 15:30
@UziTech UziTech requested a review from borisyankov as a code owner May 10, 2021 15:30
@typescript-bot
Copy link
Contributor

typescript-bot commented May 10, 2021

@UziTech Thank you for submitting this PR!

This is a live comment which I will keep updated.

1 package in this PR

Code Reviews

Because you edited one package and updated the tests (👏), I can help you merge this PR once someone else signs off on it.

Status

  • ✅ No merge conflicts
  • ✅ Continuous integration tests have passed
  • ✅ Most recent commit is approved by type definition owners or DT maintainers

All of the items on the list are green. To merge, you need to post a comment including the string "Ready to merge" to bring in your changes.


Diagnostic Information: What the bot saw about this PR
{
  "type": "info",
  "now": "-",
  "pr_number": 52004,
  "author": "UziTech",
  "headCommitOid": "e9c711edc71d080f0749dcb08cc83b918ab22cf0",
  "lastPushDate": "2021-05-10T15:22:58.000Z",
  "lastActivityDate": "2021-05-11T01:45:57.000Z",
  "maintainerBlessed": "Waiting for Code Reviews",
  "mergeOfferDate": "2021-05-10T23:38:55.000Z",
  "mergeRequestDate": "2021-05-11T01:45:57.000Z",
  "mergeRequestUser": "UziTech",
  "hasMergeConflict": false,
  "isFirstContribution": false,
  "tooManyFiles": false,
  "popularityLevel": "Critical",
  "pkgInfo": [
    {
      "name": "jasmine",
      "kind": "edit",
      "files": [
        {
          "path": "types/jasmine/index.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/jasmine/jasmine-tests.ts",
          "kind": "test"
        }
      ],
      "owners": [
        "borisyankov",
        "theodorejb",
        "davidparsson",
        "gmoothart",
        "lukas-zech-software",
        "Engineer2B",
        "cyungmann",
        "Roaders",
        "devoto13",
        "fdim",
        "kolodny",
        "stephenfarrar",
        "djungowski",
        "chivesrs",
        "kirjs",
        "ienzam"
      ],
      "addedOwners": [],
      "deletedOwners": [],
      "popularityLevel": "Critical"
    }
  ],
  "reviews": [
    {
      "type": "approved",
      "reviewer": "chivesrs",
      "date": "2021-05-10T18:04:03.000Z",
      "isMaintainer": false
    },
    {
      "type": "stale",
      "reviewer": "sgravrock",
      "date": "2021-05-10T14:46:12.000Z",
      "abbrOid": "8be00c9"
    }
  ],
  "ciResult": "pass"
}

@typescript-bot
Copy link
Contributor

🔔 @borisyankov @theodorejb @davidparsson @gmoothart @lukas-zech-software @Engineer2B @cyungmann @Roaders @devoto13 @FDIM @kolodny @stephenfarrar @djungowski @chivesrs @kirjs @ienzam — 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.

@typescript-bot
Copy link
Contributor

@sgravrock Thank you for reviewing this PR! The author has pushed new commits since your last review. Could you take another look and submit a fresh review?

@typescript-bot
Copy link
Contributor

Updated numbers for you here from 9747135.

These typings are for a version of jasmine that doesn’t yet exist on master, so I’ve compared them with v3.6.

Comparison details 📊
3.6@master 3.7 in #52004 diff
Batch compilation
Memory usage (MiB) 90.2 94.5 +4.8%
Type count 15186 15659 +3%
Assignability cache size 5418 5581 +3%
Language service
Samples taken 2887 3309 +15%
Identifiers in tests 2887 3309 +15%
getCompletionsAtPosition
    Mean duration (ms) 446.4 429.3 -3.8%
    Mean CV 6.8% 6.4%
    Worst duration (ms) 721.6 761.4 +5.5%
    Worst identifier badness foo
getQuickInfoAtPosition
    Mean duration (ms) 449.7 431.0 -4.1%
    Mean CV 7.1% 6.5%
    Worst duration (ms) 635.3 628.3 -1.1%
    Worst identifier objectContaining resolveTo
System information
Node version v14.16.0 v14.16.1
CPU count 2 2
CPU speed 2.294 GHz 2.294 GHz
CPU model Intel(R) Xeon(R) CPU E5-2673 v4 @ 2.30GHz Intel(R) Xeon(R) CPU E5-2673 v4 @ 2.30GHz
CPU Architecture x64 x64
Memory 6.8 GiB 6.8 GiB
Platform linux linux
Release 4.15.0-1111-azure 4.15.0-1113-azure

First off, note that the system varied slightly between these two runs, so you’ll have to take these measurements with a grain of salt.

It looks like nothing changed too much. I won’t post performance data again unless it gets worse.

@typescript-bot typescript-bot added Perf: Same typescript-bot determined that this PR will not significantly impact compilation performance. and removed Perf: Better typescript-bot determined that this PR improves compilation performance. labels May 10, 2021
@typescript-bot typescript-bot added the Owner Approved A listed owner of this package signed off on the pull request. label May 10, 2021
@typescript-bot typescript-bot added the Self Merge This PR can now be self-merged by the PR author or an owner label May 10, 2021
@UziTech
Copy link
Contributor Author

UziTech commented May 11, 2021

Ready to merge

@typescript-bot typescript-bot merged commit 816138e into DefinitelyTyped:master May 11, 2021
@typescript-bot
Copy link
Contributor

I just published @types/jasmine@3.7.2 to npm.

@StanislavKharchenko
Copy link

Hello!
Could you please share more info regarding removal of interfaces like CustomReporterResult, SuiteInfo, etc?
Is somewhere changelog or documenation on this?

@devoto13
Copy link
Contributor

devoto13 commented May 17, 2021

@StanislavKharchenko I believe they were renamed and slightly changed to more correctly describe the shape of objects passed to the reporter hooks (https://github.com/DefinitelyTyped/DefinitelyTyped/pull/52004/files#diff-344efb0ef17abfa10b5fd8c8152d7e2250795f7bf199d26d00bcd5e5f3f80495L894). If you use them as part of hooks it should be a matter of renaming the imports (or removing them altogether - as parameter types should be inferred automatically). Or do you have another use case for these interfaces?

@StanislavKharchenko
Copy link

@devoto13
We're using our custom reporters which inherit base hooks "jasmineStarted", "specStarted" etc and have default parameters with specified arguments and its types. Like:

public jasmineStarted(suiteInfo: jasmine.SuiteInfo): void {
    super.jasmineStarted(suiteInfo);
    //Our own iimplementation
}

And we will have compilation error since interface "SuiteInfo" doesn't exist with type jasmine.
So, we need to make changes in accordance with last interface naming.

@devoto13
Copy link
Contributor

devoto13 commented May 17, 2021

I see.

@UziTech Maybe we should also export these interfaces under old names as deprecated to ease the migration?
Properties seem to be backward compatible, so it should be pretty simple to do. E.g.

/** @deprecated use JasmineStartedInfo instead */
type SuiteInfo = JasmineStartedInfo;

/** @deprecated use SuiteResult or SpecResult instead */
type CustomReporterResult = SuiteResult & SpecResult;

/** @deprecated use JasmineStartedInfo instead */
type RunDetails = JasmineDoneInfo;

@coyoteecd
Copy link
Contributor

@UziTech @sgravrock I ended up here after doing an update of @types/jasmine, which now gives me a bunch of compile errors because of the renamed interfaces (I am using jasmine together with jasmine-spec-reporter package, and that package is now broken because of the rename).

That deprecation notice would've been great.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Critical package Owner Approved A listed owner of this package signed off on the pull request. Perf: Same typescript-bot determined that this PR will not significantly impact compilation performance. Self Merge This PR can now be self-merged by the PR author or an owner
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants