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

benchmark: skip test-benchmark-os on IBMi #50286

Closed
wants to merge 3 commits into from

Conversation

mhdawson
Copy link
Member

  • IBMi does not have the os.uptime implemented so skip otherwise CI tests fail.

- IBMi does not have the os.uptime implemented so skip
  otherwise CI tests fail.

Signed-off-by: Michael Dawson <[email protected]>
@nodejs-github-bot nodejs-github-bot added benchmark Issues and PRs related to the benchmark subsystem. needs-ci PRs that need a full CI run. performance Issues and PRs related to the performance of Node.js. test Issues and PRs related to the tests. labels Oct 19, 2023
Copy link
Contributor

@Uzlopak Uzlopak left a comment

Choose a reason for hiding this comment

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

LGTM

@mhdawson
Copy link
Member Author

CI run to confirm this skips the test on IBMi and the CI is green - https://ci.nodejs.org/view/All/job/node-test-commit-ibmi/1338/

Signed-off-by: Michael Dawson <[email protected]>
@mhdawson
Copy link
Member Author

@abmusse FYI

benchmark/run.js Outdated Show resolved Hide resolved
Signed-off-by: Michael Dawson <[email protected]>
@mhdawson
Copy link
Member Author

@H4ad @anonrig that works for me as well, pushed commit to do it that way instead.

Copy link
Contributor

@abmusse abmusse left a comment

Choose a reason for hiding this comment

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

LGTM

@mhdawson
Copy link
Member Author

CI to verify again that is passed on ibmi - https://ci.nodejs.org/view/All/job/node-test-commit-ibmi/1339/

Comment on lines +14 to +17
if (os.type() === 'OS400') {
console.log('Skipping: os.uptime is not implemented on IBMi');
process.exit(0);
}
Copy link
Member

@H4ad H4ad Oct 19, 2023

Choose a reason for hiding this comment

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

I think it is better to put outside main, on top of the file, just not to have any weird behavior.

But is not a blocker for me, so do it if you agree.

Copy link
Member

Choose a reason for hiding this comment

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

@mhdawson can you move this to a similar position like in other fs benchmarks?

@H4ad H4ad added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 19, 2023
@mhdawson mhdawson added request-ci Add this label to start a Jenkins CI on a PR. fast-track PRs that do not need to wait for 48 hours to land. labels Oct 20, 2023
@github-actions
Copy link
Contributor

Fast-track has been requested by @mhdawson. Please 👍 to approve.

@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 20, 2023
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

mhdawson added a commit that referenced this pull request Oct 24, 2023
- IBMi does not have the os.uptime implemented so skip
  otherwise CI tests fail.

Signed-off-by: Michael Dawson <[email protected]>
PR-URL: #50286
Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
@mhdawson
Copy link
Member Author

Landed in abd8ff6

@mhdawson mhdawson closed this Oct 24, 2023
alexfernandez pushed a commit to alexfernandez/node that referenced this pull request Nov 1, 2023
- IBMi does not have the os.uptime implemented so skip
  otherwise CI tests fail.

Signed-off-by: Michael Dawson <[email protected]>
PR-URL: nodejs#50286
Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
targos pushed a commit that referenced this pull request Nov 11, 2023
- IBMi does not have the os.uptime implemented so skip
  otherwise CI tests fail.

Signed-off-by: Michael Dawson <[email protected]>
PR-URL: #50286
Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
UlisesGascon pushed a commit that referenced this pull request Dec 11, 2023
- IBMi does not have the os.uptime implemented so skip
  otherwise CI tests fail.

Signed-off-by: Michael Dawson <[email protected]>
PR-URL: #50286
Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
@UlisesGascon UlisesGascon mentioned this pull request Dec 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. benchmark Issues and PRs related to the benchmark subsystem. fast-track PRs that do not need to wait for 48 hours to land. needs-ci PRs that need a full CI run. performance Issues and PRs related to the performance of Node.js. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants