Skip to content

test: skip setPriority test if insufficient permissions - #55863

Closed
aduh95 wants to merge 1 commit into
nodejs:mainfrom
aduh95:test-os-setPriority-EACCES
Closed

test: skip setPriority test if insufficient permissions#55863
aduh95 wants to merge 1 commit into
nodejs:mainfrom
aduh95:test-os-setPriority-EACCES

Conversation

@aduh95

Copy link
Copy Markdown
Contributor

@nodejs-github-botnodejs-github-bot added needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. labels Nov 15, 2024
@LiviaMedeiros

Copy link
Copy Markdown
Member

I'm not sure this is correct approach.

According to NixOS/nixpkgs#355919 (comment), the issue in NixOS happened without having a094a81? In this case, it would make sense to try cherry-picking a094a81 itself because it fixes EACCES (assuming niceness there works the same way as in Linux, and assuming it happened because the niceness in build system was set lower than 10; otherwise it looks like a bug to me).

@codecov

codecovBot commented Nov 15, 2024

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.41%. Comparing base (d9540b5) to head (f60cc58).
Report is 39 commits behind head on main.

Additional details and impacted files
@@ Coverage Diff @@## main #55863 +/- ##
==========================================
- Coverage 88.41% 88.41% -0.01% 
==========================================
Files 654 654 Lines 187811 187852 +41 Branches 36134 36130 -4 ==========================================
+ Hits 166059 166092 +33 - Misses 14992 15002 +10 + Partials 6760 6758 -2 

see 27 files with indirect coverage changes

---- 🚨 Try these New Features:

@JohnRTitorJohnRTitor left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Works on NixOS

@aduh95aduh95 added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 15, 2024
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 15, 2024
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@aduh95

Copy link
Copy Markdown
ContributorAuthor

@nodejs/fs can I get some reviews please?

@LiviaMedeirosLiviaMedeiros left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Not blocking it as-is, since there are platforms that do not allow setting lower priority (which is IMHO a bug, i don't see why it should be restricted intentionally), and assuming unbreaking their build being urgent.
But if we ignore EACCES, it's better to remove this part of test completely, because it's already covered by test-os-process-priority.js.

Comment on lines 85 to 87
if (!common.isIBMi) {
const { PRIORITY_BELOW_NORMAL, PRIORITY_LOW } = os.constants.priority;
const LOWER_PRIORITY = os.getPriority() > PRIORITY_BELOW_NORMAL ? PRIORITY_BELOW_NORMAL : PRIORITY_LOW;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
if(!common.isIBMi){
const{PRIORITY_BELOW_NORMAL,PRIORITY_LOW}=os.constants.priority;
constLOWER_PRIORITY=os.getPriority()>PRIORITY_BELOW_NORMAL ? PRIORITY_BELOW_NORMAL : PRIORITY_LOW;

Comment on lines +88 to 98
try {
os.setPriority(LOWER_PRIORITY);
const priority = os.getPriority();
is.number(priority);
assert.strictEqual(priority, LOWER_PRIORITY);
} catch (err) {
// The current user might not have sufficient permissions to set this
// specific priority level.
if (err?.info?.code !== 'EACCES') throw err;
}
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
try{
os.setPriority(LOWER_PRIORITY);
constpriority=os.getPriority();
is.number(priority);
assert.strictEqual(priority,LOWER_PRIORITY);
}catch(err){
// The current user might not have sufficient permissions to set this
// specific priority level.
if(err?.info?.code!=='EACCES')throwerr;
}
}

@aduh95

Copy link
Copy Markdown
ContributorAuthor

Superseded by #55908

@aduh95aduh95 closed this Nov 20, 2024
@aduh95
aduh95 deleted the test-os-setPriority-EACCES branch November 20, 2024 19:30
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-ciPRs that need a full CI run.testIssues and PRs related to the tests.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Build failure: nodePackages_latest.nodejs

6 participants

@aduh95@LiviaMedeiros@nodejs-github-bot@cjihrig@jazelly@JohnRTitor