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

CI: test with --with-snapshot on the slower platforms #14222

Closed
refack opened this issue Jul 13, 2017 · 7 comments
Closed

CI: test with --with-snapshot on the slower platforms #14222

refack opened this issue Jul 13, 2017 · 7 comments
Labels
build Issues and PRs related to build files or the CI. discuss Issues opened for discussions and feedbacks. flaky-test Issues and PRs related to the tests with unstable failures on the CI. performance Issues and PRs related to the performance of Node.js. test Issues and PRs related to the tests.

Comments

@refack
Copy link
Contributor

refack commented Jul 13, 2017

  • Version: 8.1.4
  • Platform: *
  • Subsystem: test,CI

Ref: #14214 (comment)
Should we run the CI tests configured --with-snapshot for the slower (Δt > 50%) platforms? Just so we get back to reasonable time frames? (node-test-commit-arm up to ~1h38m from ~29m)

/cc @nodejs/testing @nodejs/release @nodejs/build

@refack refack added build Issues and PRs related to build files or the CI. flaky-test Issues and PRs related to the tests with unstable failures on the CI. lts-agenda performance Issues and PRs related to the performance of Node.js. test Issues and PRs related to the tests. discuss Issues opened for discussions and feedbacks. labels Jul 13, 2017
@evanlucas
Copy link
Contributor

I don't think that it is a good idea to run the tests with snapshots, but release builds without snapshots.

@cjihrig
Copy link
Contributor

cjihrig commented Jul 13, 2017

I agree with @evanlucas. And, as painful as the CI may currently be, I think we should try to work through the snapshot issue before taking any drastic (maybe too strong of a word) measures.

@refack
Copy link
Contributor Author

refack commented Jul 13, 2017

Random sample Δt numbers:
#14214 (comment)
without - https://ci.nodejs.org/job/node-test-commit/11091/
with - https://ci.nodejs.org/job/node-test-commit/11045/

platform with(minutes) without(minutes) Δt good enough
node-test-commit-aix 38 42 10% ✔️
node-test-commit-arm 29 98 237% 🔴
node-test-commit-arm-fanned 35 70 100% 🔴
node-test-commit-freebsd 19 53 178% 🔴
node-test-commit-linux 27 32 18% ✔️
node-test-commit-linux-fips 30 67 123% 🔴
node-test-commit-linuxone 3 6.5 116% ✔️
node-test-commit-osx 19 25 30% ✔️
node-test-commit-plinux 8.5 24 182% 🔴
node-test-commit-smartos 16 21 131% 🔴
node-test-commit-windows-fanned 39 40 2% ✔️

@refack
Copy link
Contributor Author

refack commented Jul 13, 2017

Compromise: a check box in the submit job form?

@mhdawson
Copy link
Member

I'm in agreement that we need to test in the same way that we ship.

@refack
Copy link
Contributor Author

refack commented Jul 13, 2017

I want to make an argument for leniency.

  1. We already make some trade offs in our test suite (skipping /pummel/ and /internet/).
  2. At some point we'll want to restore the snapshot and we don't want that to rot.
  3. Building with --with-snapshot is still a valid use case for those who build from source.

tl;dr adding a checkbox (enable / disable snapshot) in the job submission form might be a good thing, by it increasing coverage.

@gibfahn
Copy link
Member

gibfahn commented Sep 16, 2017

@refack snapshots have been re-enabled so this can be closed right?

Reopen if I'm wrong.

@gibfahn gibfahn closed this as completed Sep 16, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. discuss Issues opened for discussions and feedbacks. flaky-test Issues and PRs related to the tests with unstable failures on the CI. performance Issues and PRs related to the performance of Node.js. test Issues and PRs related to the tests.
Projects
None yet
Development

No branches or pull requests

5 participants