cfv-project / cfv

Command-line File Verify
GNU General Public License v2.0
58 stars 10 forks source link

Test suite expects `/data1` to not exists #26

Closed lxp closed 3 years ago

lxp commented 3 years ago

As reported by @Terry-Kennedy, a test case fails, when /data1 exists: https://github.com/cfv-project/cfv/issues/8#issuecomment-889520893

>>> failed test: /sysprog/terry/cfv-python3/test/cfv -ZNVRMUI --unquote=no --fixpaths="" --strippaths=0 --showpaths=autorelative --progress=no --announceurl=url --noprivate_torrent --strippaths=none -T -f teststrip-none.csv4 
/data1 : file size does not match (13!=33)
teststrip-none.csv4: 1 files, 0 OK, 1 badsize.  0.000 seconds, 0.0K/s
FAILED (4)
  File "test/test.py", line 1876, in <module>
  File "test/test.py", line 1719, in all_tests
  File "test/test.py", line 269, in test_generic
  File "test/test.py", line 254, in test_log_results
  File "test/test.py", line 237, in test_log_finish

It is bad practice that the test suite makes assumptions about things outside it's own scope (i.e. temp directory). However, the test case cannot easily be adjusted without modifying it's intention. Therefore, we just change the expected test outcome, depending on if the /data1 exists or not. Leaving only an unexpected error, if someone has a file /data1 with exactly the same content as https://github.com/cfv-project/cfv/blob/python3/test/testdata/data1.

lxp commented 3 years ago

@Terry-Kennedy Can you verify, if the current python3 branch fixes the issue on your system too?

Terry-Kennedy commented 3 years ago

It does fix the issue and the test suite now runs cleanly.

But I'm confused - if the file it wants to test against is in test/testdata/data1 why does it seem to be passing /data1 (in the root of the filesystem) instead of test/testdata/data1? Note that I have a /data2 and /data3 and tests don't trip over those...

lxp commented 3 years ago

The affected test case tests the option --strippaths=none with the checksum file https://github.com/cfv-project/cfv/blob/python3/test/testdata/teststrip-none.csv4, which references /data1. This option changes the default behavior (--strippaths=0), so that the root folder or drive is not stripped.

As it is impossible to write a static positive test case for this, because the temp directory path changes with every run and we do not want to write anywhere outside that temp directory, this test case is implemented as negative test case. If the option is applied correctly, it previously expected the result not found. I changed it to check if /data1 actually exists and to expect the result bad crc in such a case.

If there would be a major implementation error with the --strippaths=none option, then one outcome could be that it strips the root folder and finds the file in test/testdata/data1, which would result in OK and, therefore, in a failed test case.

The --strippaths test cases only use data1, and not the other test files. Therefore, you don't see a problem with the others.

One additional note: If I would want to rename the test file data1, this would basically affect all tests and I would have to change or regenerate nearly all static test checksum files located in test/testdata.

I hope this explains the issue well enough. I am going to close the issue, if you still have questions regarding it, you can just write another comment or if that doesn't work just re-open the issue.