darcs

Patch 2035 extend test for issue2275 and mark (again) as failing

Title extend test for issue2275 and mark (again) as failing
Superseder Nosy List bf
Related Issues
Status accepted Assigned To
Milestone

Created on 2020-06-23.13:36:36 by bf, last changed 2020-07-22.06:09:00 by ganesh.

Files
File name Status Uploaded Type Edit Remove
extend-test-for-issue2275-and-mark-_again_-as-failing.dpatch bf, 2020-06-23.13:36:36 application/x-darcs-patch
patch-preview.txt bf, 2020-06-23.13:36:36 text/x-darcs-patch
patch-preview.txt bf, 2020-07-21.12:33:22 text/x-darcs-patch
re_enable-test-for-issue2275.dpatch bf, 2020-07-21.12:33:22 application/x-darcs-patch
unnamed bf, 2020-06-23.13:36:36 text/plain
unnamed bf, 2020-07-21.12:33:22 text/plain
See mailing list archives for discussion on individual patches.
Messages
msg22111 (view) Author: bf Date: 2020-06-23.13:36:36
1 patch for repository http://darcs.net/screened:

patch 568880a10cd72220c5fc71c8b242388a842b5462
Author: Ben Franksen <ben.franksen@online.de>
Date:   Mon Jun 22 12:08:23 CEST 2020
  * extend test for issue2275 and mark (again) as failing
Attachments
msg22183 (view) Author: ganesh Date: 2020-07-19.00:20:57
Is it worth keeping the passing ones in a non-failing script?
msg22219 (view) Author: bf Date: 2020-07-20.12:33:29
> Is it worth keeping the passing ones in a non-failing script?

This would mean duplicating the test code, so I don't think it is
worthwhile. I factored the actual tests into a shell function so we can
easily test with and without using the index. The latter succeeds, while
the former fails. Also note that normally the index is used by default.
It is only when runing the tests that we disable it because it uses only
low (1 second) resolution file modification timestamps. Fixing this is
difficult because unix-compat does not support modificationTimeHiRes, so
we'd have to roll our own for Windows.
msg22224 (view) Author: ganesh Date: 2020-07-21.00:19:07
It just seems like a step backwards.

Before we had a test that we would keep working by default, and that
validated some slightly tricky correctness property.

Now we just have a failing test and no particular plan to fix it.

We could also keep the original test as-is and put all the new code
in a new test script.
msg22228 (view) Author: bf Date: 2020-07-21.10:07:48
> It just seems like a step backwards.
> 
> Before we had a test that we would keep working by default, and that 
> validated some slightly tricky correctness property.
> 
> Now we just have a failing test and no particular plan to fix it.

It cannot be that hard to fix that. The culprit is the index code i.e.
Darcs.Util.Index. We have to review that code and find the places where
we accept symbolic links. I guess that won't affect more than a hand
full of code lines. So there is a plan, sort of, but admittedly
not yet very concrete.

I agree that loosing the parts of the test that pass is bad.

> We could also keep the original test as-is and put all the new code 
> in a new test script.

I think a better option is to just disable the part where the tests are
run with --no-ignore-times and add a comment that this currently fails.
This is pretty easy to do. I think I'll send a patch to do that.
msg22229 (view) Author: bf Date: 2020-07-21.12:33:22
Following up on review / discussion.

1 patch for repository http://darcs.net/screened:

patch b1379884b275d9152302a580573b241020489cf4
Author: Ben Franksen <ben.franksen@online.de>
Date:   Tue Jul 21 14:37:41 CEST 2020
  * re-enable test for issue2275
    
  The test fails only if the index is used, that is, if --no-ignore-times is
  in effect. This part is now temporarily disabled until we fix that.
Attachments
msg22238 (view) Author: ganesh Date: 2020-07-21.21:51:23
Good plan, thanks.
History
Date User Action Args
2020-06-23 13:36:36bfcreate
2020-06-23 14:28:52bfsetstatus: needs-screening -> needs-review
2020-07-19 00:20:57ganeshsetstatus: needs-review -> review-in-progress
messages: + msg22183
2020-07-20 12:33:30bfsetmessages: + msg22219
2020-07-21 00:19:07ganeshsetmessages: + msg22224
2020-07-21 10:07:48bfsetmessages: + msg22228
2020-07-21 12:33:22bfsetfiles: + patch-preview.txt, re_enable-test-for-issue2275.dpatch, unnamed
messages: + msg22229
2020-07-21 21:51:23ganeshsetmessages: + msg22238
2020-07-21 21:51:27ganeshsetstatus: review-in-progress -> accepted-pending-tests
2020-07-22 06:09:00ganeshsetstatus: accepted-pending-tests -> accepted