GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — build: upgrade jasmine (and related typings) to latest version by gkalpak · Pull Request #19904 · angular/angular

Conversation

gkalpak commented Oct 24, 2017 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

PR Checklist

The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit
~~Tests for the changes have been added (for bug fixes / features)~~
~~Docs have been added / updated (for bug fixes / features)~~

PR Type

[x] Build related changes

What is the current behavior?

Older version of jasmine/jasmine-node and corresponding typings.

What is the new behavior?

Latest version of jasmine/jasmine-node and corresponding typings.

Does this PR introduce a breaking change?

[ ] Yes
[x] No(t intentionally :grin:)

Other information

With these changes, the types are a little stricter now and also not compatible with Protractor's jasmine-like syntax. So, we have to also use @types/jasminewd2 for e2e tests (but not for non-e2e tests).

I also had to "augment" @types/jasminewd2, because the latest typings from DefinitelyTyped do not reflect the fact that the jasminewd2 version currently used by Protractor (v2.1.0) supports passing a done callback to a spec.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

googlebot added the cla: yes label

Oct 24, 2017

gkalpak added area: build & ci

Related the build and CI infrastructure of the project

action: review

The PR is still awaiting reviews from at least one requested reviewer

target: patch

This PR is targeted for the next patch release

PR target: TBD and removed target: patch This PR is targeted for the next patch release labels

Oct 24, 2017

mary-poppins commented Oct 24, 2017

Copy link

Copy Markdown

You can preview 3e2ed3c at https://pr19904-3e2ed3c.ngbuilds.io/.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

gkalpak force-pushed the build-upgrade-jasmine branch from 3e2ed3c to bc82db9 Compare

October 24, 2017 17:34

gkalpak commented

Oct 24, 2017

View reviewed changes

Comment thread

tools/types-ext/jasminewd2.d.ts Outdated

gkalpak Oct 24, 2017

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Come on clang-format! 😞

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

gkalpak commented

Oct 24, 2017

View reviewed changes

Comment thread

tools/types-ext/jasminewd2.d.ts Outdated

gkalpak Oct 24, 2017

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

😞 😞 😞

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

mary-poppins commented Oct 24, 2017

Copy link

Copy Markdown

You can preview bc82db9 at https://pr19904-bc82db9.ngbuilds.io/.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

gkalpak force-pushed the build-upgrade-jasmine branch from bc82db9 to c0746ed Compare

October 24, 2017 18:13

mary-poppins commented Oct 24, 2017

Copy link

Copy Markdown

You can preview c0746ed at https://pr19904-c0746ed.ngbuilds.io/.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

gkalpak force-pushed the build-upgrade-jasmine branch from c0746ed to c4c20ae Compare

October 25, 2017 07:54

gkalpak added state: WIP and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels

Oct 25, 2017

gkalpak force-pushed the build-upgrade-jasmine branch from c4c20ae to 9316f5b Compare

October 25, 2017 08:02

mary-poppins commented Oct 25, 2017

Copy link

Copy Markdown

You can preview c4c20ae at https://pr19904-c4c20ae.ngbuilds.io/.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

mary-poppins commented Oct 25, 2017

Copy link

Copy Markdown

You can preview 9316f5b at https://pr19904-9316f5b.ngbuilds.io/.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

gkalpak force-pushed the build-upgrade-jasmine branch from 9316f5b to 5a78966 Compare

October 25, 2017 09:27

mary-poppins commented Oct 25, 2017

Copy link

Copy Markdown

You can preview 5a78966 at https://pr19904-5a78966.ngbuilds.io/.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

gkalpak commented

Oct 25, 2017

View reviewed changes

Comment thread

packages/common/test/pipes/date_pipe_spec.ts Outdated

gkalpak Oct 25, 2017

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

This relies on timezone (e.g. failed on my UTC+2 timezone 😞).
Using 12 as the hour it is more resilient to timezone differences (albeit not entirely immune) and thus more likely to pass when run locally.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

ocombe Oct 25, 2017 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

yes, I had to change the match to use a regexp that doesn't check the last values, but I didn't think about the hour that could go to the other day. It's fine to use 12 instead of 22

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

gkalpak commented

Oct 25, 2017

View reviewed changes

Comment thread

packages/compiler-cli/test/metadata/collector_spec.ts Outdated

gkalpak Oct 25, 2017

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I noticed that there are several occurrences where types were a little loose or not-specific in compiler-cli-related stuff. For example nullable/"undefined-able" values typed as non-nullable (e.g. here, here, here) or values whose properties could be more strictly typed (e.g. the changes in this file), etc.

Is this expected/intentional, @tbosch, @chuckjaz?
_{Obviously, I don't understand enough about the implications of changing the types of these APIs 😁

Just bringing it up as my instinctive, very-high-level-understanding reaction/thought.}

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

chuckjaz Oct 25, 2017

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Can you leave TODO review comments anywhere where as any as is needed?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

gkalpak reacted with thumbs up emoji

All reactions

👍 1 reaction

gkalpak commented

Oct 25, 2017

View reviewed changes

Comment thread

packages/core/test/linker/source_map_integration_node_only_spec.ts Outdated

gkalpak Oct 25, 2017

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

@tbosch, @chuckjaz: Not a big deal, but if it is possible to retrieve the JIT sources without spying on such a fundamental built-in (Function), I think it is worth doing. (The current approach is a little brittle.)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

mhevery Jul 2, 2018

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Can you add comment // Work around for: https://github.com/jasmine/jasmine/issues/1573

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

gkalpak reacted with thumbs up emoji

All reactions

👍 1 reaction

gkalpak commented

Oct 25, 2017

View reviewed changes

Comment thread

packages/router/src/url_tree.ts Outdated

gkalpak Oct 25, 2017 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

This is a public API change ({key: string} --> {key: any}) plus using the already public Params type for that.
But I think it is necessary, because this is what actually happens (i.e. the values are not restricted to string).

If a more specific type is desirable, I can dig deeper into what the possible types are and update (from a quick look it might just be string | string[]).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

IgorMinar Nov 2, 2017

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

@jasonaden will have to take a look at this. it looks like a breaking change to me, which we can't do now. Jason is this the same api change you were trying to make during v5 RC?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

gkalpak Nov 2, 2017

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Just to be clear, the public API type is wrong. It claims that queryParams values are always strings, but this is not the case. So, your app might typecheck fine, but break at runtime. But this change might indeed cause an app not to compile any more (for good reasons 😉)

LMK if it is better to remove it (and use as any in the tests to work around it).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

gkalpak Nov 29, 2017

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Just to be more clear, Params is alias for {[key: string]: any}, so it is wider than the previous type.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

gkalpak commented

Oct 25, 2017

View reviewed changes

Comment thread

tools/types-ext/jasminewd2.d.ts Outdated

gkalpak Oct 25, 2017

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

@vikerman (or anyone else working on jasminewd), should these changes be ported to DefinitelyTyped?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

alexeagle Oct 25, 2017

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Yes, you can send me a PR against the upstream typings on DefinitelyTyped, I'm a committer there

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

gkalpak reacted with thumbs up emoji

All reactions

👍 1 reaction

gkalpak force-pushed the build-upgrade-jasmine branch from 5a78966 to 8a3b7da Compare

October 25, 2017 11:17

gkalpak changed the title ~~build: upgrade jasmine to latest version~~ build: upgrade jasmine (and related typings) to latest version

Oct 25, 2017

mary-poppins commented Oct 25, 2017

Copy link

Copy Markdown

You can preview 8a3b7da at https://pr19904-8a3b7da.ngbuilds.io/.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

gkalpak force-pushed the build-upgrade-jasmine branch from 8a3b7da to 4e2adb2 Compare

October 25, 2017 23:06

gkalpak added 3 commits

July 5, 2018 21:53


          test: make NgMatchers type-aware

0b9619d


          refactor: infer type for it() assertion functions

d9b5d4a


          build: upgrade jasmine to 2.99.x and fix tests

af521c3

gkalpak force-pushed the build-upgrade-jasmine branch 2 times, most recently from 1fdac95 to 97dfaf3 Compare

July 5, 2018 19:07

mary-poppins commented Jul 5, 2018

Copy link

Copy Markdown

You can preview 1fdac95 at https://pr19904-1fdac95.ngbuilds.io/.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

mary-poppins commented Jul 5, 2018

Copy link

Copy Markdown

You can preview 97dfaf3 at https://pr19904-97dfaf3.ngbuilds.io/.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

gkalpak force-pushed the build-upgrade-jasmine branch from 97dfaf3 to 0f35833 Compare

July 5, 2018 19:45

mary-poppins commented Jul 6, 2018

Copy link

Copy Markdown

You can preview 0f35833 at https://pr19904-0f35833.ngbuilds.io/.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.


          refactor: re-organize and "modernize" cjs-jasmine scripts

a1fa87d

gkalpak force-pushed the build-upgrade-jasmine branch from 0f35833 to 1a3f52c Compare

There was an error while loading. Please reload this page.

gkalpak added 3 commits

July 6, 2018 12:51


          test: run unit tests in random order

f83f603


          build: upgrade jasmine to 3.1.0

964a122


          build: upgrade karma and related dependencies

0ebba85

mary-poppins commented Jul 6, 2018

Copy link

Copy Markdown

You can preview 0ebba85 at https://pr19904-0ebba85.ngbuilds.io/.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

gkalpak commented Jul 6, 2018

Copy link

Copy Markdown

Member Author

This is now ready for review 😅
(cc @mhevery, @rkirov)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

ngbot Bot commented Jul 6, 2018

Copy link

Copy Markdown

I see that you just added the PR action: merge label, but the following checks are still failing:
     forbidden label detected: PR action: review
     status "code-review/pullapprove" is pending
     status "google3" is pending

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

mhevery commented Jul 6, 2018 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor

http://test/OCL:203500801:BASE:203501165:1530900687238:5ce75398

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

alfaproject reviewed

Jul 6, 2018

View reviewed changes

Comment thread

packages/benchpress/test/webdriver/chrome_driver_extension_spec.ts

		*/

		import {AsyncTestCompleter, describe, expect, iit, inject, it} from '@angular/core/testing/src/testing_internal';
		import {AsyncTestCompleter, describe, expect, fit, inject, it} from '@angular/core/testing/src/testing_internal';

alfaproject Jul 6, 2018

All reactions

👍 1 reaction

alfaproject reviewed

Jul 6, 2018

View reviewed changes

Comment thread

packages/common/http/test/client_spec.ts

		*/

		import {ddescribe, describe, iit, it} from '@angular/core/testing/src/testing_internal';
		import {ddescribe, describe, fit, it} from '@angular/core/testing/src/testing_internal';

googlebot commented Jul 6, 2018

Copy link

Copy Markdown

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

googlebot commented Jul 6, 2018

Copy link

Jul 10, 2018

Revert #24803 on branch 6.0.x #24809

Closed

gkalpak mentioned this pull request

Sep 19, 2018

[jasminewd2] Add DoneFn to supporting functions DefinitelyTyped/DefinitelyTyped#28957

Merged

7 of 9 tasks

angular-automatic-lock-bot Bot commented Sep 13, 2019

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.}

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Reviewers

8 more reviewers

alfaproject

alfaproject left review comments

mhevery

mhevery left review comments

chuckjaz

chuckjaz left review comments

alexeagle

alexeagle approved these changes

IgorMinar

IgorMinar approved these changes

ocombe

ocombe approved these changes

jasonaden

jasonaden approved these changes

hansl

hansl approved these changes

Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

mhevery

IgorMinar

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: major

This PR is targeted for the next major release

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

12 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

Conversation

gkalpak commented Oct 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

Uh oh!

mary-poppins commented Oct 24, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mary-poppins commented Oct 24, 2017

Uh oh!

mary-poppins commented Oct 24, 2017

Uh oh!

mary-poppins commented Oct 25, 2017

Uh oh!

mary-poppins commented Oct 25, 2017

Uh oh!

mary-poppins commented Oct 25, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ocombe Oct 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gkalpak Oct 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mary-poppins commented Oct 25, 2017

Uh oh!

mary-poppins commented Jul 5, 2018

Uh oh!

mary-poppins commented Jul 5, 2018

Uh oh!

mary-poppins commented Jul 6, 2018

Uh oh!

mary-poppins commented Jul 6, 2018

Uh oh!

mary-poppins commented Jul 6, 2018

Uh oh!

mary-poppins commented Jul 6, 2018

Uh oh!

mary-poppins commented Jul 6, 2018

Uh oh!

gkalpak commented Jul 6, 2018

Uh oh!

ngbot Bot commented Jul 6, 2018

Uh oh!

mhevery commented Jul 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

gkalpak commented Oct 24, 2017 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

ocombe Oct 25, 2017 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

gkalpak Oct 25, 2017 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

mhevery commented Jul 6, 2018 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.