ftpd returns 0 for all timestamps #1688

New Issue

Instead of the constant 626644800 in expected_root, use a class attribute, e.g. self.FALL_OF_THE_BERLIN_WALL.
"if name not in children: raise [NoSuchChildError](wiki/NoSuchChildError)(name)" in _modifier is not covered, and not needed (it's ok to let it raise a KeyError on childrenname).
The case where "linkmotime" is in the metadata is not tested. (I saw this by looking at the test coverage [*] and seeing that the relevant line "value = metadata["tahoe"]["linkmotime"]" of source:src/allmydata/frontends/ftpd.py was not run. Of course many other lines of ftpd.py are also not tested yet, but this is a line added by the fix.)
It is not tested that a file added via FTP (rather than by the internal Python API) is created with the right timestamp. This ticket can be closed without that test as long as it is documented on #512 that it is needed.
Add a comment above the relevant line in expected_root saying that the timestamp is expected to be 0 if no timestamp metadata is present.

[*] wiki/HowToWriteTests , which is currently being written, will describe how to generate test coverage reports.

These look excellent, thanks. Comments: * Instead of the constant 626644800 in `expected_root`, use a class attribute, e.g. `self.FALL_OF_THE_BERLIN_WALL`. * "`if name not in children: raise [NoSuchChildError](wiki/NoSuchChildError)(name)`" in `_modifier` is not covered, and not needed (it's ok to let it raise a `KeyError` on `childrenname`). * The case where `"linkmotime"` is in the metadata is not tested. (I saw this by looking at the test coverage [*] and seeing that the relevant line "`value = metadata["tahoe"]["linkmotime"]`" of source:src/allmydata/frontends/ftpd.py was not run. Of course many other lines of `ftpd.py` are also not tested yet, but this is a line added by the fix.) * It is not tested that a file added via FTP (rather than by the internal Python API) is created with the right timestamp. This ticket can be closed without that test as long as it is documented on #512 that it is needed. * Add a comment above the relevant line in `expected_root` saying that the timestamp is expected to be 0 if no timestamp metadata is present. [*] [wiki/HowToWriteTests](wiki/HowToWriteTests) , which is currently being written, will describe how to generate test coverage reports.

lebek commented

2012-03-31 00:08:11 +00:00

Attachment fix_ftpd_mtime_with_unit_test.dpatch (90334 bytes) added

------WebKitFormBoundarytFYJYBMb7621074p
Content-Disposition: form-data; name="replace"

on

**Attachment** fix_ftpd_mtime_with_unit_test.dpatch (90334 bytes) added ------WebKitFormBoundarytFYJYBMb7621074p Content-Disposition: form-data; name="replace" on

fix_ftpd_mtime_with_unit_test.dpatch

88 KiB

lebek commented

2012-03-31 00:10:18 +00:00

Thanks for the review davidsarah.

Replying to davidsarah:

Instead of the constant 626644800 in expected_root, use a class attribute, e.g. self.FALL_OF_THE_BERLIN_WALL.

Done, also added a self.TURN_OF_MILLENIUM in order to differentiate the output in the case where both mtime and linkmotime are set.

"if name not in children: raise [NoSuchChildError](wiki/NoSuchChildError)(name)" in _modifier is not covered, and not needed (it's ok to let it raise a KeyError on childrenname).

Agreed.

The case where "linkmotime" is in the metadata is not tested. (I saw this by looking at the test coverage [*] and seeing that the relevant line "value = metadata["tahoe"]["linkmotime"]" of source:src/allmydata/frontends/ftpd.py was not run. Of course many other lines of ftpd.py are also not tested yet, but this is a line added by the fix.)

Fixed.

It is not tested that a file added via FTP (rather than by the internal Python API) is created with the right timestamp. This ticket can be closed without that test as long as it is documented on #512 that it is needed.

I have added a note to #512.

Add a comment above the relevant line in expected_root saying that the timestamp is expected to be 0 if no timestamp metadata is present.

Done.

Thanks for the review davidsarah. Replying to [davidsarah](/tahoe-lafs/trac-2024-07-25/issues/1688#issuecomment-87749): > * Instead of the constant 626644800 in `expected_root`, use a class attribute, e.g. `self.FALL_OF_THE_BERLIN_WALL`. Done, also added a `self.TURN_OF_MILLENIUM` in order to differentiate the output in the case where both `mtime` and `linkmotime` are set. > * "`if name not in children: raise [NoSuchChildError](wiki/NoSuchChildError)(name)`" in `_modifier` is not covered, and not needed (it's ok to let it raise a `KeyError` on `childrenname`). Agreed. > * The case where `"linkmotime"` is in the metadata is not tested. (I saw this by looking at the test coverage [*] and seeing that the relevant line "`value = metadata["tahoe"]["linkmotime"]`" of source:src/allmydata/frontends/ftpd.py was not run. Of course many other lines of `ftpd.py` are also not tested yet, but this is a line added by the fix.) Fixed. > * It is not tested that a file added via FTP (rather than by the internal Python API) is created with the right timestamp. This ticket can be closed without that test as long as it is documented on #512 that it is needed. I have added a note to #512. > * Add a comment above the relevant line in `expected_root` saying that the timestamp is expected to be 0 if no timestamp metadata is present. Done.

davidsarah commented

2012-03-31 00:46:08 +00:00

Excellent! Good tests.

tahoe-lafs added

normal

and removed

minor

labels 2012-03-31 00:46:08 +00:00

tahoe-lafs modified the milestone from undecided to 1.9.2

2012-03-31 00:46:08 +00:00

davidsarah commented

2012-03-31 01:23:35 +00:00

Note that fix_ftpd_size_type.dpatch is equivalent to warner's patch on #680, which we dithered about applying for three years ~~because of an incompatibility with one FTP client, curlftpfs~~ because we misunderstood a different bug exposed by the fix. We eventually decided to apply it, but then missed the 1.9.0 deadline.

Note that [fix_ftpd_size_type.dpatch](/tahoe-lafs/trac-2024-07-25/attachments/000078ac-8b89-a871-65fe-c69a6b31b1cb) is equivalent to warner's patch on #680, which we dithered about applying for three years ~~because of an incompatibility with one FTP client, curlftpfs~~ because we misunderstood a different bug exposed by the fix. We eventually decided to apply it, but then missed the 1.9.0 deadline.

tahoe-lafs added the

fixed

label 2012-03-31 02:10:27 +00:00

davidsarah closed this issue

2012-03-31 02:10:27 +00:00

davidsarah commented

2012-03-31 02:21:32 +00:00

Fixed by changeset:14a50f258a0cb249, changeset:be1fd9d2b501ade9 and changeset:7f6ee7e9180377bc.

davidsarah commented

2012-03-31 02:24:39 +00:00

with a pyflakes fix in changeset:74cfa65f0d35ea12.

Sign in to join this conversation.