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

Expand coverage of v8 testing from Node.js tree #387

Closed
mhdawson opened this issue Apr 13, 2016 · 18 comments
Closed

Expand coverage of v8 testing from Node.js tree #387

mhdawson opened this issue Apr 13, 2016 · 18 comments
Labels

Comments

@mhdawson
Copy link
Member

mhdawson commented Apr 13, 2016

#199 and related PR's added v8 testing on the version of v8 in the node tree (which is different from what is tested by google due to our floating patches).

We currently have a CI run that can be used to test proposed changes as well as a nightly run:
https://ci.nodejs.org/job/node-test-commit-v8-linux/

However, we don't test against all platforms/architectures. This issue is to track adding those additional combinations as need, h/w and or time allows.

Architectures we could/are testing include:

Arch Covered Notes
x64 Yes
Arm No Should work but don't have beefy enough hardware
PPC le Yes
PPC be Yes
s390 Yes

O/Ss that we could/are testing include:

OS Covered Notes
linux Yes
mac No Should work but don't have enough hardware
windows No Needs work on build scripts
smartos No Needs investigation of machine issues
freebsd No Not tested, likely to work but did not think we needed to
AIX No Should work but don't have enough hardware

We also only test on x64 which we believe is ok as we think the risk of our floating patches only affecting 32 bit is low. If this proves not to be the case we can adjust.

@mhdawson
Copy link
Member Author

@orangemocha, @misterdjules FYI in case you want to help out getting these running on windows/smartos

@mhdawson mhdawson added the build label Apr 13, 2016
@misterdjules
Copy link
Contributor

@mhdawson Thank you very much for the heads up, very much appreciated! How can I get access to the SmartOS machines that are used to run these tests?

Also, I thought we were trying to avoid floating patches on top of V8 as much as possible. Do we have a list of V8 patches that are currently floating on top of each node branch?

@jbergstroem
Copy link
Member

As I outlined in the other issue, we need to reprovision a bigger vm to test on SmartOS. I can look at doing that in the coming week. As for windows, I reckon @joaocgreis would be the best guy to decide which machine we can use (or if we have to reprovision).

@misterdjules
Copy link
Contributor

@jbergstroem Excellent, I'll let you handle this, feel free to ping me if you need anything. Thank you!

@bnoordhuis
Copy link
Member

We also only test on x64 which we believe is ok as we think the risk of our floating patches only affecting 32 bit is low. If this proves not to be the case we can adjust.

I'd be happier if we tested both, even if we only do it initially on e.g. x86 and x86_64 Linux. V8 is largely platform agnostic but it's very much not architecture agnostic.

I'm very happy with the progress we're making, by the way!

@mhdawson
Copy link
Member Author

@jbergstroem is there a x86 32 bit machine that is big enough to run the tests ?

@mhdawson
Copy link
Member Author

@misterdjules "trying to avoid floating" is the key part. While we trying to keep the list small I think its quite likely we will have some small number at any one point.

@jbergstroem
Copy link
Member

@mhdawson There's PAE and swap. Reckon both would work.

@misterdjules
Copy link
Contributor

@mhdawson Is there a list of these floating patches somewhere so that we know at least how close we are from that goal?

@mhdawson
Copy link
Member Author

@misterdjules I don't have the lis, @ofrobots might comment as he put together a recent PR for 5.0. I don't think we have a process for capturing them in a list but I have recent a recent one related to the test-tick-process on mac so I know there still are some. I also think that even if we reach that goal we'll likely to have temporary ones as we wait for changes to percolate into new releases (and in the LTS releases were we won't take new v8 levels)

@mhdawson
Copy link
Member Author

@jbergstroem sorry I'm not quite following. I searched on the ci page for machines with PAE or swap in the name and did not find anything but maybe I'm just blind.

@misterdjules
Copy link
Contributor

@mhdawson

I don't have the list, @ofrobots might comment as he put together a recent PR for 5.0. I don't think we have a process for capturing them in a list but I have recent a recent one related to the test-tick-process on mac so I know there still are some.

Sounds good. We had such a list in joyent/node and at that time I thought it was helpful. The fact that it was maintained manually was not ideal, but just having that info publicly available for everyone interested is I think valuable.

@ofrobots
Copy link

@misterdjules @mhdawson At the moment we don't have a list of floating patches. I manually look at this history of the deps/v8 directory.

I do agree that a list would be nice, however, the V8 team is talking to the LTS WG to figure out if V8 can unfreeze old branches so that Node could use those directly instead of floating patches. That might be an even nicer solution than keeping a manually updated list.

@refack
Copy link
Contributor

refack commented Aug 18, 2017

A possiblly related case where a patch made the V8 CI jobs fail, while the node jobs pass:
nodejs/node#14913.
IMHO simply adding node-test-commit-v8-linux/nodes=rhel72-s390x,v8test=v8test (it's an 8 minute job) to the node-test-commit matrix will cover these cases.

@mhdawson
Copy link
Member Author

@refack node-test-commit-v8-linux/nodes=rhel72-s390x was already there in the job, I just had not updated the matrix in the first comment in this issue. See https://ci.nodejs.org/job/node-test-commit-v8-linux/ to see the 4 platforms covered.

@refack
Copy link
Contributor

refack commented Aug 18, 2017

@mhdawson Ack, but it's not in the node-test-commit cascade, so what might have happened is that a PR landed on node/master that passes node-test-commit but breaks node-test-commit-v8-linux.
(my assumption is that D8 doesn't compile, and it's not part of the "regular" node build tree):

../src/d8.cc:229:12: error: 'platform_' was not declared in this scope
     return platform_->GetTracingController();

@mhdawson
Copy link
Member Author

@refack got it. I thought you were asking that it be added. Is it that you were suggesting that we add the v8 tests to the node-test-commit cascade ? That is different from the topic of this issue but might be an idea to explore. We've not done that in the past because of how long it runs, but we might be able to add it for zlinux since it runs fast.

@Trott
Copy link
Member

Trott commented Mar 22, 2019

Closing due to long period of inactivity. Feel free to re-open if this is a thing. I'm just trying to close stuff that has been ignored for sufficiently long that it seems likely it's not something we're going to get to.

@Trott Trott closed this as completed Mar 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

7 participants