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.

Sign up for GitHub

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

Jump to bottom

test: remove Object.observe from tests #4769

Closed
wants to merge 1 commit into from
Closed

test: remove Object.observe from tests #4769

wants to merge 1 commit into from

Conversation

vkurchatkin
Copy link
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
Copy link
Contributor

Do we know when v8 will drop it?

@silverwind 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
Copy link
Contributor

cjihrig commented Jan 19, 2016

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

@vkurchatkin
Copy link
Contributor Author

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

@silverwind
Copy link
Contributor

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

Optimistically targetting M50 at the moment.

@mgol
Copy link
Contributor

mgol commented Jan 19, 2016

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
Copy link
Contributor

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

@cjihrig
Copy link
Contributor

cjihrig commented Jan 20, 2016

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

@Fishrock123
Copy link
Contributor

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

@bnoordhuis
Copy link
Member

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

@bnoordhuis
Copy link
Member

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

@cjihrig
Copy link
Contributor

cjihrig commented Jan 20, 2016

Makes sense then. LGTM

@mscdex
Copy link
Contributor

mscdex commented Jan 20, 2016

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

@jasnell
Copy link
Member

jasnell commented Jan 22, 2016

They appear to be unrelated.
LGTM

@silverwind
Copy link
Contributor

Thanks! Landed in b4313cf.

@silverwind silverwind closed this Jan 27, 2016
silverwind pushed a commit that referenced this pull request Jan 27, 2016
@vkurchatkin @silverwind
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
Copy link
Member

ChALkeR commented Jan 27, 2016

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
@vkurchatkin @rvagg
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
@vkurchatkin @rvagg
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 rvagg mentioned this pull request Feb 8, 2016
Closed
MylesBorins pushed a commit that referenced this pull request Feb 17, 2016
@vkurchatkin
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
@vkurchatkin
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 MylesBorins mentioned this pull request Feb 18, 2016
Merged
MylesBorins pushed a commit that referenced this pull request Mar 2, 2016
@vkurchatkin
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
@vkurchatkin
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>
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
@vkurchatkin @silverwind @cjihrig @mgol @Fishrock123 @bnoordhuis @mscdex @jasnell @ChALkeR @MylesBorins