patchGNU Octave - Patches: patch #9148, Added some BIST test for...

 
 

patch #9148: Added some BIST test for isinteger() in libinterp/corefcn/data.cc

Submitter:  NVS Abhilash <nvs232>
Submitted:  Thu 20 Oct 2016 05:10:19 PM UTC
   
 
Category:  Core : other Priority:  5 - Normal
Status:  Done Privacy:  Public
Assigned to:  None Open/Closed:  Closed
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Thu 10 Nov 2016 10:37:04 PM UTC, comment #7: 

Thank you. The code looks great now. I only made a couple minor fixes to the commit message. I added a period at the end of the line, and I added the "F" prefix to "isinteger" because all of the functions that are declared as DEFUNs have a C++ symbol with an "F" prefix.

I pushed your patch on the stable branch, it will be part of Octave 4.2 coming in a matter of days. Thank you for your contribution!

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

Mike Miller <mtmiller>
Group Member
Sat 05 Nov 2016 09:30:24 AM UTC, comment #6: 

Hey Mike,

I am really sorry for wasting your time on this, I completely understand that It was all my job to find out how to commit and how to write the assert statements. I am sorry I was a bit of ignorant. I should have researched more. I am sorry for that. I will do a better research about anything I am doing next.

For now, I have again tried to do as you suggested, added asterisk character in front of my commit, added the improvised asserts for the false statements.

If any problem, please let me know.

Thanks,
NVS

(file #38893)

NVS Abhilash <nvs232>
Tue 01 Nov 2016 10:05:42 PM UTC, comment #5: 

Yes, it doesn't look like you changed the commit message from the last one (file #38781). Please compare yours with this commit message for example: http://hg.savannah.gnu.org/hgweb/octave/rev/dc9039163760

You could use the single-line style as you have done, but you are missing the asterisk character at the start of the line in that case.

You also still have some tests that are


%!assert (expression, false)


These should instead be written as


%!assert (! expression)


(Like I said, numerous examples of these kinds of tests throughout the Octave code tree for you to refer to)

Mike Miller <mtmiller>
Group Member
Tue 01 Nov 2016 06:18:40 PM UTC, comment #4: 

Thanks Mike,

I did what you asked for and attached the diff file for the same here. If any problem, please do convey me. Also do tell me weather I did the commit the right way or not.

Thanks,
NVS

(file #38856)

NVS Abhilash <nvs232>
Mon 31 Oct 2016 06:00:38 PM UTC, comment #3: 

Sorry for the delay. Can you please squash your commits into one patch instead of making a new commit that changes the previous? I would like to see one patch at the end of this review process that can apply cleanly to the stable branch of Octave.

The commit message is not quite right, please take a look at the suggestions, and search the commit log for similar changes that add BIST tests, for example

http://hg.savannah.gnu.org/hgweb/octave/search/?rev=add%20BIST&revcount=80
http://hg.savannah.gnu.org/hgweb/octave/rev/dc9039163760

Mike Miller <mtmiller>
Group Member
Fri 21 Oct 2016 06:18:24 AM UTC, comment #2: 

Thanks Mike, now I have attached the new version of the diff, in which I

1. edited to remove the true values.
2. I also replaced single quotes to double quotes.
3. I also did new commits keeping the commit guidelines.

If their is any other problem in the patch please tell me.

As a side note I did't find a case where false value from the assert can be removed. Please do convey me if there exists a better way to implement it.

Thank you.

(file #38781)

NVS Abhilash <nvs232>
Thu 20 Oct 2016 06:15:16 PM UTC, comment #1: 

This patch applies cleanly and the tests are written mostly correctly. Some changes are needed before it can be applied:

  • prefer double quoted strings to single quotes whenever possible
  • when a function returns a true or false value, assert should be called with only one argument instead of comparing it to "true" or "false" (there are numerous examples of this in existing unit tests)
  • follow the commit message guildelines (http://wiki.octave.org/Commit_message_guidelines)



Mike Miller <mtmiller>
Group Member
Thu 20 Oct 2016 05:10:19 PM UTC, original submission:  

I added some BIST for isinteger() function present in libinterp/corefcn/data.cc. Attached is the diff file, for the same.

isinteger is one of many functions which lacked Built In Self Tests. This patch is an attempt to give a complete set of test cases for the function isinteger.

For any feedback or wueries mail me at -email is unavailable-

NVS Abhilash <nvs232>

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #38893:  isinteger.diff added by nvs232 (2KiB - text/x-patch - Changed commit message and improved assert calls.)
file #38856:  isinteger.diff added by nvs232 (2KiB - text/x-patch - Added a single diff, which could directly be added to the main source.)
file #38781:  isinteger3.diff added by nvs232 (2KiB - text/x-patch - Removed default true values, and replace single quotes to double quotes.)
file #38776:  isinteger.diff added by nvs232 (2KiB - text/x-patch - Added is the diff file where isinteger BIST are added)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by mtmiller (Posted a comment)
  • -email is unavailable- added by nvs232 (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 6 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2016-11-10 mtmiller StatusNone Done
        Open/ClosedOpen Closed
    2016-11-05 nvs232 Attached File- Added isinteger.diff, #38893
    2016-11-01 nvs232 Attached File- Added isinteger.diff, #38856
    2016-10-21 nvs232 Attached File- Added isinteger3.diff, #38781
    2016-10-20 nvs232 Attached File- Added isinteger.diff, #38776

    Back to the top

    Powered by Savane 3.13-02a9.
    Corresponding source code