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

If the disk does not support hdparm -C, use unknown state #46

Closed
wants to merge 6 commits into from
Closed

If the disk does not support hdparm -C, use unknown state #46

wants to merge 6 commits into from

Conversation

prehor
Copy link
Contributor

@prehor prehor commented Aug 6, 2023

hdparm -C on WD SSD Black SN750 M.2 returns no output.

`hdparm -C` on WD SSD Black SN750 M.2 does not return no output
hddfancontrol/__init__.py Outdated Show resolved Hide resolved
hddfancontrol/__init__.py Outdated Show resolved Hide resolved
@desbma
Copy link
Owner

desbma commented Aug 13, 2023

I have a WD_BLACK SN850 that outputs for hdparm -C:

/dev/nvme0:
 drive state is:  unknown

What is the exact output you get?

@prehor
Copy link
Contributor Author

prehor commented Aug 13, 2023

I have a WD_BLACK SN850 that outputs for hdparm -C:

/dev/nvme0:
 drive state is:  unknown

What is the exact output you get?

On Debian Buster with kernel 4, hdparm -C /dev/nvme0 exits with an error message.
On Alpine Linux with kernel 6, hdparm -C /dev/nvme0 exits with the same message as yours.
In both cases it returns errno = ENOTTY(25) and the exception needs to be caught.

hddfancontrol/__init__.py Outdated Show resolved Hide resolved
@@ -194,9 +194,8 @@ def test_getState(self):
universal_newlines=True,
)
with unittest.mock.patch("hddfancontrol.subprocess.check_output") as subprocess_check_output_mock:
subprocess_check_output_mock.side_effect = subprocess.CalledProcessError(0, "")
Copy link
Owner

Choose a reason for hiding this comment

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

This side effect should be kept, we are testing that a hdparm error leads to an hddfancontrol.Drive.DriveState.UNKNOWN state.

I suggest (untested):

            subprocess_check_output_mock.side_effect = subprocess.CalledProcessError(0, "")
            self.assertEqual(self.drive.getState(), hddfancontrol.Drive.DriveState.UNKNOWN)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed. Please re-run the test because most of them failed with error:

coveralls.exception.CoverallsException: Could not submit coverage: 422 Client Error: Unprocessable Entity for url: https://coveralls.io/api/v1/jobs

@@ -244,9 +243,8 @@ def test_isSleeping(self):
universal_newlines=True,
)
with unittest.mock.patch("hddfancontrol.subprocess.check_output") as subprocess_check_output_mock:
subprocess_check_output_mock.side_effect = subprocess.CalledProcessError(0, "")
Copy link
Owner

@desbma desbma Sep 18, 2023

Choose a reason for hiding this comment

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

Same remark as above, and this a test for isSleeping so we should be testing that, not getState.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@coveralls
Copy link

Coverage Status

coverage: 39.474% (+0.4%) from 39.075%
when pulling f837574 on prehor:prehor-patch-3
into 6cd2143 on desbma:master.

@desbma
Copy link
Owner

desbma commented Dec 17, 2023

Rebased, squashed and merged separately in f74a475

@desbma desbma closed this Dec 17, 2023
@prehor prehor deleted the prehor-patch-3 branch December 29, 2023 15:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants