Hello,

I've attached a patch for issue 2348, which is to implement cabal's
test-suite.

Ryan

Attachments

• issue2348.dpatch

Thanks for working on this!

I realised after we discussed it on IRC that a bit more cleanup is needed to switch:

 - The "test" flag should be removed from the earlier bits of the cabal file (the
cabal option --enable-tests is the replacement for -ftest)

 - remove the custom hook from Setup.lhs - it's doing a bit of work there but I think the 
default behaviour of the darcs-test executable is probably adequate.

Would you be able to look at those?

In terms of what you've done so far, I have a couple of comments:

 - Did you have to remove the build-tools line? I think it is valid in a test-suite, though I'm 
not sure if it buys us anything given that there's another copy in the main section of the cabal 
file.

 - Also your patch description should be a bit more descriptive of what the patch does. If you 
start it with "resolve issue2348: .." then the bug tracker will recognise that automatically. So 
perhaps something like "resolve issue2348: switch to cabal's test framework".

You can use darcs amend-record to edit the patch description and to add further changes to it.

Hello Ganesh,

I've done the following:

* pulled changes from last night
* removed test flag from darcs.cabal  (test pass as normal)
* removed runtest from Setup.lhs 
   - cabal-test had a different output
      ...
      Linking dist/build/darcs-test/darcs-test ...
      Running 1 test suites...
      Test suite darcs-test: RUNNING...
      Test suite darcs-test: PASS
      Test suite logged to: dist/test/darcs-2.9.8-darcs-test.log
      1 of 1 test suites (1 of 1 test cases) passed.

* cleaned up Setup.lhs based off hlint suggestions
* working on adding runTest to harness/test.hs

I'll also amend the record so that it's more descriptive.  I can roll
these other changes into the record as well, right?

Ryan

On 11/26/2013 06:34 PM, Ganesh Sittampalam wrote:
> Ganesh Sittampalam <ganesh@earth.li> added the comment:
>
> Thanks for working on this!
>
> I realised after we discussed it on IRC that a bit more cleanup is needed to switch:
>
>  - The "test" flag should be removed from the earlier bits of the cabal file (the
> cabal option --enable-tests is the replacement for -ftest)
>
>  - remove the custom hook from Setup.lhs - it's doing a bit of work there but I think the 
> default behaviour of the darcs-test executable is probably adequate.
>
> Would you be able to look at those?
>
> In terms of what you've done so far, I have a couple of comments:
>
>  - Did you have to remove the build-tools line? I think it is valid in a test-suite, though I'm 
> not sure if it buys us anything given that there's another copy in the main section of the cabal 
> file.
>
>  - Also your patch description should be a bit more descriptive of what the patch does. If you 
> start it with "resolve issue2348: .." then the bug tracker will recognise that automatically. So 
> perhaps something like "resolve issue2348: switch to cabal's test framework".
>
> You can use darcs amend-record to edit the patch description and to add further changes to it.
>
> ----------
> assignedto:  -> rdesfo
> issues: +Please use the new testing interface instead
> nosy: +ganesh
> status: needs-screening -> followup-requested
>
> __________________________________
> Darcs bug tracker <bugs@darcs.net>
> <http://bugs.darcs.net/patch1108>
> __________________________________
>

Hi Ryan,

That's great, thanks.

Yes, you can both update the description and add more contents using
amend-record, and it makes sense to do so here.

Cheers,

Ganesh

On 27/11/2013 00:52, Ryan wrote:
> Hello Ganesh,
> 
> I've done the following:
> 
> * pulled changes from last night
> * removed test flag from darcs.cabal  (test pass as normal)
> * removed runtest from Setup.lhs 
>    - cabal-test had a different output
>       ...
>       Linking dist/build/darcs-test/darcs-test ...
>       Running 1 test suites...
>       Test suite darcs-test: RUNNING...
>       Test suite darcs-test: PASS
>       Test suite logged to: dist/test/darcs-2.9.8-darcs-test.log
>       1 of 1 test suites (1 of 1 test cases) passed.
> 
> * cleaned up Setup.lhs based off hlint suggestions
> * working on adding runTest to harness/test.hs
> 
> I'll also amend the record so that it's more descriptive.  I can roll
> these other changes into the record as well, right?
> 
> Ryan
> 
> On 11/26/2013 06:34 PM, Ganesh Sittampalam wrote:
>> Ganesh Sittampalam <ganesh@earth.li> added the comment:
>>
>> Thanks for working on this!
>>
>> I realised after we discussed it on IRC that a bit more cleanup is needed to switch:
>>
>>  - The "test" flag should be removed from the earlier bits of the cabal file (the
>> cabal option --enable-tests is the replacement for -ftest)
>>
>>  - remove the custom hook from Setup.lhs - it's doing a bit of work there but I think the 
>> default behaviour of the darcs-test executable is probably adequate.
>>
>> Would you be able to look at those?
>>
>> In terms of what you've done so far, I have a couple of comments:
>>
>>  - Did you have to remove the build-tools line? I think it is valid in a test-suite, though I'm 
>> not sure if it buys us anything given that there's another copy in the main section of the cabal 
>> file.
>>
>>  - Also your patch description should be a bit more descriptive of what the patch does. If you 
>> start it with "resolve issue2348: .." then the bug tracker will recognise that automatically. So 
>> perhaps something like "resolve issue2348: switch to cabal's test framework".
>>
>> You can use darcs amend-record to edit the patch description and to add further changes to it.
>>
>> ----------
>> assignedto:  -> rdesfo
>> issues: +Please use the new testing interface instead
>> nosy: +ganesh
>> status: needs-screening -> followup-requested
>>
>> __________________________________
>> Darcs bug tracker <bugs@darcs.net>
>> <http://bugs.darcs.net/patch1108>
>> __________________________________
>>
> 
>

Thanks for the updated patch - looks good.

The use of default-language prompts a cabal warning saying we should 
require Cabal-Version >= 1.10 ; I made an attempt to make that change 
but found a swamp of changes to do with how we declare our language 
extensions, so have held off on that for now. I'll send a followup to 
comment out default-language for not, unless there's a strong reason we 
need to keep it.

Attachments

• resolve-issue2348_-switch-to-cabal_s-test-framework.dpatch

This patch bundle (with 1 patches) was just applied to the repository http://darcs.net/screened.
This message was brought to you by DarcsWatch
http://darcswatch.nomeata.de/repo_http:__darcs.net_screened.html#bundle-86ece657805c80ea203376ab61225920907d1337

I think we can unstuck this from the pipeline, and we fix the
default-language bug later.

This patch bundle (with 1 patches) was just applied to the repository http://darcs.net/reviewed.
This message was brought to you by DarcsWatch
http://darcswatch.nomeata.de/repo_http:__darcs.net_reviewed.html#bundle-86ece657805c80ea203376ab61225920907d1337