★ wanayoo — archive 1999 https://github.com/nodejs/node/pull/26943Nouvelle recherche | Portail wanayoo
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.

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

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

Closed
wants to merge 1 commit into from

Conversation

danbev
Copy link
Member

@danbev danbev commented Mar 27, 2019

…onstructor"

This reverts commit ed5e69d.

Refs: #26850 (comment)

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

@nodejs-github-bot
Copy link
Contributor

@nodejs-github-bot nodejs-github-bot commented Mar 27, 2019

@BridgeAR
Copy link
Member

@BridgeAR BridgeAR commented Mar 27, 2019

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

@joyeecheung joyeecheung commented Mar 27, 2019

+1 to fast track

@danbev
Copy link
Member Author

@danbev danbev commented Mar 27, 2019

@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
Copy link
Member

@refack refack commented Mar 27, 2019

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

@joyeecheung joyeecheung commented Mar 27, 2019

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

@BridgeAR
Copy link
Member

@BridgeAR BridgeAR commented Mar 27, 2019

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

refack
refack approved these changes Mar 27, 2019
@BridgeAR
Copy link
Member

@BridgeAR BridgeAR commented Mar 27, 2019

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

@refack refack commented Mar 27, 2019

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
Copy link
Member Author

@danbev danbev commented Mar 27, 2019

Landed in a8eac78.

@danbev danbev closed this Mar 27, 2019
danbev added a commit that referenced this issue Mar 27, 2019
…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>
@refack refack added the landed label Mar 27, 2019
@targos targos added this to Don't land (ever) in v11.x Mar 27, 2019
@BridgeAR
Copy link
Member

@BridgeAR BridgeAR commented Apr 23, 2019

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

This revert should probably be reverted again.

BridgeAR added a commit to BridgeAR/node that referenced this issue Apr 29, 2019
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 added a commit that referenced this issue Apr 30, 2019
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 Aug 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
v11.x
  
Don't land (ever)
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants