Closed mmuetzel closed 1 year ago
Can you test this change because I don't have a windows machine?
Can you test this change because I don't have a windows machine?
Why would you need a Windows machine?
On my systems, this test passes OK. So I can't recreate the error failed you mentioned above.
I can push the changes you recommend so you can test them if it is more convenient for you.
Does it work for you if you install the package in a read-only location? Do the tests still pass with the change?
I don't know how to do this, ie install the package in a read only location.
You could install the package, check where it was installed (e.g., using the information from pkg prefix
) and set the directory containing that .cc-tst file to read-only.
After that, run pkg test statistics
.
Don't forget to re-set to read-write permissions after the test.
I just did what you suggested and it still passes the tests.
Summary:
PASS 4044
FAIL 0
XFAIL (reported bug) 1
XFAIL (expected failure) 1
With Octave 8.0.90:
>> pkg load statistics
>> cd 'C:/Program Files/GNU Octave/Octave-8.0.90/mingw64/lib/octave/packages/statistics-1.5.2/x86_64-w64-mingw32-api-v57+'
>> [L, D] = libsvmread (file_in_loadpath ("heart_scale.dat"));
>> libsvmwrite ("filename", [L;L], D);
error: libsvmwrite: error opening file for write.
>> cd 'C:\Users\Markus'
>> libsvmwrite ("filename", [L;L], D);
error: libsvmwrite: length of label vector does not match instances.
>> cd 'C:/Program Files/GNU Octave/Octave-8.0.90/mingw64/lib/octave/packages/statistics-1.5.2/x86_64-w64-mingw32-api-v57+'
>> libsvmwrite (fullfile (tempdir (), "filename"), [L;L], D);
error: libsvmwrite: length of label vector does not match instances.
>>
So, using the full path to the test file in the temporary directory seems to make the test emit the expected error message.
I also noticed that the test doesn't delete the file after it is done. The file is created even if the function errors out:
>> exist (fullfile (tempdir (), "filename"), "file")
ans = 2
It would be good if it were deleted after the test is done.
Maybe, that is also the reason why you couldn't reproduce: It might be that the file already existed for you from a previous run in the directory with the .cc-tst
file (from when the directory had still read-write permission).
Maybe, that made the test pass for you.
You are right, the file was present from previous testing. I have updated the code in the BISTs and pushed the changes. Now it runs properly when the folder is read only. I am not sure whether it is required to delete the "filename" file in the temp folder, but I added a line to delete it anyway. Please, run the test with the latest commit so we can close this issue, if everything is ok with your version of Octave
Made additional changes in the code so that the file stream is closed and the file is deleted if the
Since those tests for errors can actually overwrite an existing file, it might be better to use tempname ()
instead of the generic name fullfile (tempdir (), "filename")
. (I wasn't aware that those error tests could create or overwrite a file when I suggested the latter.)
Pushed the changes in BISTs here
I don't see any changes to the BISTs in the commit you linked to.
Edit: You might have meant https://github.com/gnu-octave/statistics/commit/37dc0d808281a39f27dc92cbd71d40315dc45c07?
Edit 2: That change looks reasonable to me. 👍
Yep! wrong copy paste of the link.
A test in
libsvmwrite.cc-tst
is failing when runningpkg test statistics
if the package is installed in a read-only location:The corresponding error in the
fntests.log
file:This was noticed with the version of the package that is installed with Octave for Windows. But the same would likely happen for any platform when the package is installed in a read-only location.
This is probably because the current directory is changed to the folder containing the test. Writing to the current folder will fail if that folder is read-only.
Currently, those tests look like this:
It might be better if the test file would be created in the temporary directory. The tests could, e.g., be changed to this:
I haven't tested that change though.