Skip to content

build: update jasmine to 3.5#34625

Closed
JiaLiPassion wants to merge 3 commits into
angular:masterfrom
JiaLiPassion:jasmine-3.5
Closed

build: update jasmine to 3.5#34625
JiaLiPassion wants to merge 3 commits into
angular:masterfrom
JiaLiPassion:jasmine-3.5

Conversation

@JiaLiPassion

@JiaLiPassion JiaLiPassion commented Jan 3, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • angular.io application / infrastructure changes
  • Other... Please describe:

Update jasmine to 3.5, so I can continue to work with #33657, also need to wait #33717 typescript 3.7 PR to merge first, otherwise, it will not compile.

@JiaLiPassion JiaLiPassion requested a review from a team January 3, 2020 05:29
@JiaLiPassion JiaLiPassion requested a review from a team January 3, 2020 08:51
@JiaLiPassion JiaLiPassion requested a review from a team January 3, 2020 11:30
@atscott atscott added the area: build & ci Related the build and CI infrastructure of the project label Jan 9, 2020
@ngbot ngbot Bot added this to the needsTriage milestone Jan 9, 2020
mhevery
mhevery reviewed Mar 6, 2020
View reviewed changes

@mhevery mhevery left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see this is still WIP

@JiaLiPassion

JiaLiPassion commented Mar 7, 2020

Copy link
Copy Markdown
Contributor Author

@mhevery , yeah, this is still WIP, I will finish this one asap.

@JiaLiPassion JiaLiPassion force-pushed the jasmine-3.5 branch 7 times, most recently from 0de64a5 to 0d84546 Compare March 30, 2020 08:54
@JiaLiPassion JiaLiPassion added target: major This PR is targeted for the next major release and removed state: WIP labels Mar 30, 2020
@JiaLiPassion

JiaLiPassion commented Mar 30, 2020

Copy link
Copy Markdown
Contributor Author

@mhevery, I updated the PR, some case may need to wait for the @types/jasmine update, but most cases are ok for now, and not only the jasmine and @types/jasmine are updated, some previous wrong cases are also be found, such as https://github.com/angular/angular/pull/34625/files#diff-8783fc8fad0324187d972850b203c93cR84-R89, this case will not pass, I am not sure how to fix this one, could you take a look? Thanks.

@googlebot

This comment has been minimized.

Sign in to view
@JiaLiPassion

JiaLiPassion commented Apr 2, 2020

Copy link
Copy Markdown
Contributor Author

@gkalpak , got it, thank you for the explanation.

@JiaLiPassion JiaLiPassion force-pushed the jasmine-3.5 branch 2 times, most recently from 3e821b4 to 7b12dfa Compare April 5, 2020 10:06
@mhevery mhevery added the action: merge The PR is ready for merge by the caretaker label Apr 6, 2020
@ngbot

ngbot Bot commented Apr 6, 2020

Copy link
Copy Markdown

I see that you just added the PR action: merge label, but the following checks are still failing:
    failure conflicts with base branch "master"
    pending status "pullapprove" is pending
    pending 4 pending code reviews

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

atscott
atscott approved these changes Apr 6, 2020
View reviewed changes

@atscott atscott left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed-for: fw-router

@pullapprove pullapprove Bot requested a review from atscott April 6, 2020 21:27
@AndrewKushnir AndrewKushnir removed their request for review April 6, 2020 21:42
@AndrewKushnir AndrewKushnir added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Apr 6, 2020
@AndrewKushnir

AndrewKushnir commented Apr 6, 2020

Copy link
Copy Markdown
Contributor

Hi,

Based on PullApprove, I was added as a reviewer on behalf of fw-core. Misko (also a member of fw-core) already approved this PR, so my LGTM is not needed, so I removed myself from the list of reviewers.

Note: I've added "cleanup" label because there is a couple files that conflict with master branch.

Thank you.

@JiaLiPassion JiaLiPassion removed the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Apr 6, 2020
atscott
atscott approved these changes Apr 7, 2020
View reviewed changes

@atscott atscott left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed-for: fw-router

@IgorMinar IgorMinar added target: patch This PR is targeted for the next patch release and removed target: major This PR is targeted for the next major release labels Apr 7, 2020
IgorMinar
IgorMinar approved these changes Apr 7, 2020
View reviewed changes

@IgorMinar IgorMinar left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @JiaLiPassion !

I'm relabeling this as master & patch because this change doesn't affect Angular applications.

@JiaLiPassion I doubt that this change will cherry-pick cleanly to 9.1.x branch - can you please send a separate PR against that branch? thanks!

@IgorMinar

IgorMinar commented Apr 7, 2020

Copy link
Copy Markdown
Contributor

@JiaLiPassion please rebase this PR one more time - I just spoke with @kara and we'll merge this PR as soon as it's rebased.

@kara kara added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Apr 7, 2020
JiaLiPassion and others added 3 commits April 8, 2020 11:46
@JiaLiPassion
build: update jasmine to 3.5
2955071 
1. update jasmine to 3.5
2. update @types/jasmine to 3.5
3. update @types/jasminewd2 to 2.0.8

Also fix several cases, the new jasmine 3 will help to create test cases correctly,
such as in the `jasmine 2.x` version, the following case will pass

```
expect(1 == 2);
```

But in jsamine 3, the case will need to be

```
expect(1 == 2).toBeTrue();
```
@gkalpak @JiaLiPassion
fixup! build: update jasmine to 3.5
245ce3f
@JiaLiPassion
fix(zone.js): add issue numbers of @types/jasmine to the test cases
c717df3 
Some cases will still need to use `spy as any` cast, because `@types/jasmine` have some issues,
1. The issue jasmine doesn't handle optional method properties, DefinitelyTyped/DefinitelyTyped#43486
2. The issue jasmine doesn't handle overload method correctly, DefinitelyTyped/DefinitelyTyped#42455
@JiaLiPassion

JiaLiPassion commented Apr 8, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@IgorMinar , thanks for the review, I just rebased, and I created a PR #36501 target 9.1.x branch, please review, thanks.

@JiaLiPassion JiaLiPassion mentioned this pull request Apr 8, 2020
Closed
@angular-automatic-lock-bot

angular-automatic-lock-bot Bot commented May 9, 2020

Copy link
Copy Markdown

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@atscott atscott atscott approved these changes

@alxhub alxhub alxhub approved these changes

+4 more reviewers

@mhevery mhevery mhevery approved these changes

@IgorMinar IgorMinar IgorMinar approved these changes

@kyliau kyliau kyliau approved these changes

@gkalpak gkalpak gkalpak approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@JiaLiPassion JiaLiPassion

Labels

action: merge The PR is ready for merge by the caretaker area: build & ci Related the build and CI infrastructure of the project cla: yes target: patch This PR is targeted for the next patch release

Projects

None yet

Milestone

needsTriage

Development

Successfully merging this pull request may close these issues.

10 participants

@JiaLiPassion @googlebot @gkalpak @AndrewKushnir @IgorMinar @mhevery @atscott @kyliau @alxhub @kara