Skip to content
This repository has been archived by the owner on Oct 15, 2020. It is now read-only.

src,test: fix V8 engine issues #519

Merged
merged 1 commit into from
Apr 23, 2018
Merged

src,test: fix V8 engine issues #519

merged 1 commit into from
Apr 23, 2018

Conversation

kfarnung
Copy link
Contributor

@kfarnung kfarnung commented Apr 21, 2018

  • Remove gyp include that no longer exists upstream
  • Conditionally include v8-debug.h
  • Fix test failures for V8 engine
  • Remove status entries for missing tests
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@kfarnung
Copy link
Contributor Author

Copy link
Contributor

@jackhorton jackhorton left a comment

Choose a reason for hiding this comment

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

Looks reasonable, but whats the motivation here?

@@ -23,7 +23,6 @@ test-esm-loader-modulemap : SKIP
test-esm-main-lookup : SKIP
test-esm-named-exports : SKIP
test-esm-namespace : SKIP
test-esm-ok : SKIP
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this test just removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like it was moved in fddcd62.

@kfarnung
Copy link
Contributor Author

@jackhorton I occasionally like to run a V8 build of the node-chakracore code and make sure the tests all pass. This ensures that we keep compatibility with V8. All of these issues (minus the status file cleanup) were preventing the V8 build from passing successfully.

@kfarnung kfarnung force-pushed the v8fixes branch 2 times, most recently from a52421c to 13d33ff Compare April 23, 2018 00:01
@kfarnung
Copy link
Contributor Author

@kfarnung
Copy link
Contributor Author

Ran tests on Windows and Linux:

> .\vcbuild.bat v8 test-ci ignore-flaky lint
$ ./configure --engine=v8
$ make -j4 test

No failures

Copy link
Contributor

@sethbrenith sethbrenith left a comment

Choose a reason for hiding this comment

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

:shipit:

kfarnung added a commit to kfarnung/node-chakracore that referenced this pull request Apr 23, 2018
* Remove gyp include that no longer exists upstream
* Conditionally include `v8-debug.h`
* Fix test failures for V8 engine
* Remove status entries for missing tests

PR-URL: nodejs#519
Reviewed-By: Seth Brenith <sethb@microsoft.com>
Reviewed-By: Jimmy Thomson <jithomso@microsoft.com>
@kfarnung
Copy link
Contributor Author

One more CI before landing, the last one had a bunch of benchmark test timeouts on macOS: https://ci.nodejs.org/job/chakracore-test-pull-request/243/

* Remove gyp include that no longer exists upstream
* Conditionally include `v8-debug.h`
* Fix test failures for V8 engine
* Remove status entries for missing tests

PR-URL: nodejs#519
Reviewed-By: Seth Brenith <sethb@microsoft.com>
Reviewed-By: Jimmy Thomson <jithomso@microsoft.com>
@kfarnung kfarnung merged commit bcf99c5 into nodejs:master Apr 23, 2018
kfarnung added a commit that referenced this pull request Apr 23, 2018
* Remove gyp include that no longer exists upstream
* Conditionally include `v8-debug.h`
* Fix test failures for V8 engine
* Remove status entries for missing tests

PR-URL: #519
Reviewed-By: Seth Brenith <sethb@microsoft.com>
Reviewed-By: Jimmy Thomson <jithomso@microsoft.com>
@kfarnung kfarnung deleted the v8fixes branch April 23, 2018 19:04
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants