Skip to content

Revert "console: use consolePropAttributes for k-bind properties in c… - #26943

Closed
danbev wants to merge 1 commit into
nodejs:masterfrom
danbev:revert-26850
Closed

Revert "console: use consolePropAttributes for k-bind properties in c…#26943
danbev wants to merge 1 commit into
nodejs:masterfrom
danbev:revert-26850

Conversation

@danbev

@danbevdanbev commented Mar 27, 2019

Copy link
Copy Markdown
Contributor

…onstructor"

This reverts commit ed5e69d.

Refs: #26850 (comment)

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@danbevdanbev added the fast-track PRs that do not need to wait for 48 hours to land. label Mar 27, 2019
@nodejs-github-bot

nodejs-github-bot commented Mar 27, 2019

Copy link
Copy Markdown
Collaborator

@danbev build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/3067/pipeline

@nodejs-github-botnodejs-github-bot added the console Issues and PRs related to the console subsystem. label Mar 27, 2019
@nodejs-github-bot

nodejs-github-bot commented Mar 27, 2019

Copy link
Copy Markdown
Collaborator

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

@BridgeAR

BridgeAR commented Mar 27, 2019

Copy link
Copy Markdown
Member

The failed test case it flaky and Travis is only an indicator about things working or not. I don't think that this should be reverted / that it's related.

@joyeecheung

joyeecheung commented Mar 27, 2019

Copy link
Copy Markdown
Member

+1 to fast track

@danbev

danbev commented Mar 27, 2019

Copy link
Copy Markdown
ContributorAuthor

@BridgeAR Ah good to know that it's flakey, thanks!

I'm a little confused about this issue as it does seem to be consistent on master when I run it locally and after reverting it did not fail (though I did not run it more than twice).
I'll let others chime in as well and see what people think is the best path forward.

@refack

refack commented Mar 27, 2019•
edited
Loading

Copy link
Copy Markdown
Contributor

The nightly master run in Jenkins shows the current code works on many platforms:
https://ci.nodejs.org/job/node-daily-master/1489/

Ref to flake investigation: #26401

@joyeecheung

joyeecheung commented Mar 27, 2019

Copy link
Copy Markdown
Member

We can find out by reverting it and see if it flakes again. If it flakes, revert the revert.

@BridgeAR

BridgeAR commented Mar 27, 2019

Copy link
Copy Markdown
Member

@danbev it's also reproducible for me. I am puzzled that it seems to make the test fail reliable. @addaleax PTAL.

@BridgeAR

BridgeAR commented Mar 27, 2019

Copy link
Copy Markdown
Member

To get the test at least pass again on most systems, it's probably fine to revert it for now. It just does not seem to be the actual issue to me.

@refack

refack commented Mar 27, 2019

Copy link
Copy Markdown
Contributor

I am puzzled that it seems to make the test fail reliable.

@gireeshpunathil was asking for a core dump in #26401, if anyone could accommodate...

tl;dr I'm +1 on reverting to get master stable again, then use the previous flaky code to chase down the underlying bug.

@danbev

danbev commented Mar 27, 2019

Copy link
Copy Markdown
ContributorAuthor

Landed in a8eac78.

@danbevdanbev closed this Mar 27, 2019
danbev added a commit that referenced this pull request Mar 27, 2019
@danbev
Revert "console: use consolePropAttributes for k-bind properties in c…
a8eac78
…onstructor"
This reverts commit ed5e69d.
PR-URL: #26943
Refs: #26850 (comment)
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
@BridgeAR

BridgeAR commented Apr 23, 2019

Copy link
Copy Markdown
Member

This did not fix the issue as described here: #26401 (comment)

This revert should probably be reverted again.

This was referenced Apr 23, 2019
Closed
Closed
Closed
Closed
Closed
This was referenced Apr 23, 2019
Closed
Closed
Closed
Closed
Closed
BridgeAR added a commit to BridgeAR/node that referenced this pull request Apr 29, 2019
@BridgeAR
console: use consolePropAttributes for k-bind properties (reland)
6bfde6a
This is a reland of nodejs#26850.
It was speculatively reverted but it turned out that this did not
cause any trouble.
PR-URL: nodejs#27352
Refs: nodejs#26943
Refs: nodejs#26850
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos pushed a commit that referenced this pull request Apr 30, 2019
@BridgeAR@targos
console: use consolePropAttributes for k-bind properties (reland)
8e876e6
This is a reland of #26850.
It was speculatively reverted but it turned out that this did not
cause any trouble.
PR-URL: #27352
Refs: #26943
Refs: #26850
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@danbev
danbev deleted the revert-26850 branch August 20, 2019 07:55
@baby636baby636 mentioned this pull request Jul 12, 2023
Open
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cjihrigcjihrigcjihrig approved these changes
@joyeecheungjoyeecheungjoyeecheung approved these changes
+1 more reviewer
@refackrefackrefack approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

consoleIssues and PRs related to the console subsystem.fast-trackPRs that do not need to wait for 48 hours to land.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@danbev@nodejs-github-bot@BridgeAR@joyeecheung@refack@cjihrig@targos