patchGNU Octave - Patches: patch #7934, Adding tests for operator...

 
 

patch #7934: Adding tests for operator overloading in the legacy class system (bug #38128)

Submitter:  Julien Bect <jbect>
Submitted:  Mon 28 Jan 2013 07:47:56 AM UTC
   
 
Category:  None Priority:  5 - Normal
Status:  Done Privacy:  Public
Assigned to:  None Open/Closed:  Closed
* Mandatory Fields

Add a New Comment Rich Markup
   

Mon 28 Jan 2013 09:08:26 PM UTC, comment #4: 

The quote about preserving Matlab compatibility and the "legacy" term were already there in test_classes.m (by Robert T. Short ?).

By the way, you forgot one occurence of "legacy" in the commit message ;-)

Julien Bect <jbect>
Mon 28 Jan 2013 06:50:28 PM UTC, comment #3: 


> Also, I think that assert(isequal()) is clearer than the
> two-argument syntax, but that's obviously a matter of taste (since
> you call it "ugly").


It's clearer if you're used to Matlab code. Everywhere else in Octave
we use the two- or three-arg assert call when writing tests. After
you've been writing Octave code for a while, Matlab code looks
progressively uglier. ;-)

I also disagree with the comment that you quote above that. Why should
we make this testing on Matlab easier? We don't make this concession
for any other tests in Octave.

Regardless, these are all minor stylistic issues. I may just fix them
later. I have rebased your patch to default, where it's
uncontroversial to add new things, and I've pushed it:

    http://hg.savannah.gnu.org/hgweb/octave/rev/f75ffcc82acb

If we later decide that this should indeed go on stable, we can graft
the patch back into stable.

I have also modified the metadata of your patch. I used your full
name and email as the username of the committer. I also added a commit
message in accordance with these guidelines:

    http://wiki.octave.org/Commit_message_guidelines

I changed the word "legacy". The old-style classes aren't "legacy", at
least not in Octave yet, since we don't have any replacement for them
yet. I furthermore also removed trailing whitespace. If you enable the
colour extension in Mercurial, you will see the trailing whitespace
when you do "hg diff":

    http://mercurial.selenic.com/wiki/ColorExtension

You may wish to remove your local pre-rebased version of the patch by
doing


hg strip -r 56fcf4903a2e4


then pulling the new version from Savannah.

Thanks for the patch.

Jordi GutiƩrrez Hermoso <jordigh>
Group Member
Mon 28 Jan 2013 05:28:23 PM UTC, comment #2: 

Concerning the first point: yes, I did that because Matlab doesn't have a two-argument version of assert. It was explicitely stated just above the two lines that you quote:


%%  Note: This script and all classes are also intended to run
%%        in MATLAB to test compatibility.  Don't break that!


Also, I think that assert(isequal()) is clearer than the two-argument syntax, but that's obviously a matter of taste (since you call it "ugly").

For the second point: I have no idea which branch it belongs to, please put it wherever you think it fits.

Julien Bect <jbect>
Mon 28 Jan 2013 04:39:33 PM UTC, comment #1: 

Can you please explain these lines?


+%%        Note(JBect, 2013/01/27) : in order to ease to process of testing
+%%        Matlab compatibility, the syntax assert(observed, expected) should
+%%        be avoided. I use assert(isequal(observed, expected) instead.


We don't generally care if Octave code runs in Matlab. Tests are just Octave code. Using ugly syntax just to please Matlab is very much not what we generally do. Are you just doing this because Matlab doesn't have a two-argument version of assert?

Furthermore,  I'm not sure about your decision to put these tests on stable. It may be ok. After all, they are just tests. On the other hand, we try to be extremely conservative about patching on stable, and new tests could potentially break packagers' build scripts if they expect a clean-ish exit from "make check".

Jordi GutiƩrrez Hermoso <jordigh>
Group Member
Mon 28 Jan 2013 07:47:56 AM UTC, original submission:  

This patch adds tests for operator overloading in the legacy class system. Four of these tests are currently failing (see bug #38128).

Julien Bect <jbect>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

Attached Files

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by jordigh (Posted a comment)
  • -email is unavailable- added by jbect (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only logged-in users can vote.

     

    Follow 4 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2013-01-28 jordigh StatusNeed Info Done
        Open/ClosedOpen Closed
    2013-01-28 jordigh StatusNone Need Info
    2013-01-28 jbect Attached File- Added AddingTestsLegacyClassSystem.diff, #27335

    Back to the top

    Powered by Savane 3.13-4448.
    Corresponding source code