FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

test: remove Object.observe from tests by vkurchatkin · Pull Request #4769 · nodejs/node · GitHub

/ node Public

test: remove Object.observe from tests - #4769

Closed
vkurchatkin wants to merge 1 commit into
nodejs:masterfrom
vkurchatkin:remove-observe
ConversationCommits (1)ChecksFiles changed
Closed

test: remove Object.observe from tests#4769
vkurchatkin wants to merge 1 commit into
nodejs:masterfrom
vkurchatkin:remove-observe

Conversation

vkurchatkin commented Jan 19, 2016

Copy link
Copy Markdown
Contributor

Testing this wasn't really useful, besides Object.observe is going to be deprecated.

Also this test fails with Chakra (#4765) for obvious reason.

silverwind commented Jan 19, 2016

Copy link
Copy Markdown
Contributor

Do we know when v8 will drop it?

silverwind added v8 engine Issues and PRs related to the V8 dependency. test Issues and PRs related to the tests. labels Jan 19, 2016

cjihrig commented Jan 19, 2016

Copy link
Copy Markdown
Contributor

LGTM if the CI is happy, although we should probably keep the tests as long as our V8 ships Object.observe().

vkurchatkin commented Jan 19, 2016

Copy link
Copy Markdown
Contributor Author

@cjihrig we could, but to tell the truth, it doesn't really add value in this particular case

silverwind commented Jan 19, 2016

Copy link
Copy Markdown
Contributor

Found it: https://groups.google.com/a/chromium.org/forum/#!topic/blink-reviews-bindings/vwyJiq5u38E

Optimistically targetting M50 at the moment.

mgol commented Jan 19, 2016

Copy link
Copy Markdown
Contributor

Found it: https://groups.google.com/a/chromium.org/forum/#!topic/blink-reviews-bindings/vwyJiq5u38E

Optimistically targetting M50 at the moment.

It says about deprecating, not dropping, though.

silverwind commented Jan 20, 2016

Copy link
Copy Markdown
Contributor

How about skipping the test if Object.observe isn't present? It'll likely be there a long time in v8.

cjihrig commented Jan 20, 2016

Copy link
Copy Markdown
Contributor

Is there any real rush to get rid of this? Is it hindering anything at all?

Fishrock123 commented Jan 20, 2016

Copy link
Copy Markdown
Contributor

@cjihrig It fails in Chakra because uh, Chakra doesn't have it since it's no-longer spec.

bnoordhuis commented Jan 20, 2016

Copy link
Copy Markdown
Member

LGTM. It's non-spec so let's get rid of it.

bnoordhuis commented Jan 20, 2016

Copy link
Copy Markdown
Member

CI: https://ci.nodejs.org/job/node-test-pull-request/1318/

cjihrig commented Jan 20, 2016

Copy link
Copy Markdown
Contributor

Makes sense then. LGTM

mscdex commented Jan 20, 2016

Copy link
Copy Markdown
Contributor

LGTM assuming the two test failures on pi1 in CI are unrelated.

jasnell commented Jan 22, 2016

Copy link
Copy Markdown
Member

They appear to be unrelated.
LGTM

silverwind commented Jan 27, 2016

Copy link
Copy Markdown
Contributor

Thanks! Landed in b4313cf.

silverwind closed this Jan 27, 2016
silverwind pushed a commit that referenced this pull request Jan 27, 2016
test: remove Object.observe from tests
b4313cf 
Testing this wasn't really useful, besides Object.observe is going to be
deprecated.

Also this test fails with Chakra (#4765) for obvious reason.

PR-URL: #4769
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>

ChALkeR commented Jan 27, 2016

Copy link
Copy Markdown
Member

Do we know when v8 will drop it?

I was under an impression that they already did drop it, but it seems they re-enabled it again due to backwards compatibility with something.

rvagg pushed a commit that referenced this pull request Jan 28, 2016
test: remove Object.observe from tests
959e051 
Testing this wasn't really useful, besides Object.observe is going to be
deprecated.

Also this test fails with Chakra (#4765) for obvious reason.

PR-URL: #4769
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
rvagg pushed a commit that referenced this pull request Feb 8, 2016
test: remove Object.observe from tests
f7ed473 
Testing this wasn't really useful, besides Object.observe is going to be
deprecated.

Also this test fails with Chakra (#4765) for obvious reason.

PR-URL: #4769
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
rvagg mentioned this pull request Feb 8, 2016
Closed
MylesBorins pushed a commit that referenced this pull request Feb 17, 2016
test: remove Object.observe from tests
78fc5e3 
Testing this wasn't really useful, besides Object.observe is going to be
deprecated.

Also this test fails with Chakra (#4765) for obvious reason.

PR-URL: #4769
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
MylesBorins pushed a commit that referenced this pull request Feb 18, 2016
test: remove Object.observe from tests
9e6f363 
Testing this wasn't really useful, besides Object.observe is going to be
deprecated.

Also this test fails with Chakra (#4765) for obvious reason.

PR-URL: #4769
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
MylesBorins mentioned this pull request Feb 18, 2016
Merged
MylesBorins pushed a commit that referenced this pull request Mar 2, 2016
test: remove Object.observe from tests
c490464 
Testing this wasn't really useful, besides Object.observe is going to be
deprecated.

Also this test fails with Chakra (#4765) for obvious reason.

PR-URL: #4769
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
test: remove Object.observe from tests
7225c8c 
Testing this wasn't really useful, besides Object.observe is going to be
deprecated.

Also this test fails with Chakra (nodejs#4765) for obvious reason.

PR-URL: nodejs#4769
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

test Issues and PRs related to the tests. v8 engine Issues and PRs related to the V8 dependency.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

10 participants

Back | FazBrowse Home | New Git URL