bugGNU Octave - Bugs: bug #59121, jsonencode needs functional BIST...

 
 

bug #59121: jsonencode needs functional BIST tests for case when "PrettyWriter" is true

Submitter:  Rik <rik5>
Submitted:  Thu 17 Sep 2020 02:48:21 PM UTC
   
 
Category:  Test Suite Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Feature Request
Status:  Fixed Assigned to:  None
Originator Name:  Open/Closed:  * Closed
Release:  * dev Operating System:  * Any
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Thu 24 Sep 2020 11:57:05 PM UTC, comment #6: 

I checked in two tests for "PrettyWriter" just to make sure the option works in this changeset: https://hg.savannah.gnu.org/hgweb/octave/rev/92495ca48bd7.

Marking as fixed and closing report.

Rik <rik5>
Group administrator
Fri 18 Sep 2020 10:48:20 PM UTC, comment #5: 

@Abdallah: Your reasoning makes sense.  I don't think we need to verify the differences in RapidJSON between the "Writer" class and the "PrettyWriter" class.

However, I still think we need a few tests just to verify that the "PrettyWriter" option works in Octave.  For example, what if the input validation were wrong and even when the "PrettyWriter" argument was given it dispatched to the "Writer" class?  This testing doesn't need to be extensive.

Rik <rik5>
Group administrator
Fri 18 Sep 2020 10:13:12 PM UTC, comment #4: 

Hello everyone,

Since those functions were my GSoC project with Octave, I can supply more info.

> You're right that this probably needs some more comments.  I think what the programmer had in mind is that this script could be run under Matlab to verify that the results were equivalent in Octave.  To do so, the m-file should be left in Matlab syntax so '%' for comment character, no specialized 'end' blocks like 'endif', etc.

 
That's right. Actually, The tests were originally written in MATLAB then converted to Octave's BIST since MATLAB compatibility was a goal of my project. We (me and my mentors) decided to leave it in external files so that we can check if the compatibility is broken in the newer versions of MATLAB.

Regarding the need for "PrettyWriter" tests, I didn't add tests for it because:

  • The only difference between it and the "normal" writer is the adding of some indentations and new lines but the content is basically the same so the tests seemed to trivial to me.
  • This difference is made internally in "RapidJSON" and not in a code written in Octave and I think it is not a good idea to test for things that we didn't write. RapidJSON may change the "pretty" format in the future releases and those tests will fail.


However, If you think that we still need them, I would be happy to help with that.









Abdallah Khaled Elshamy <abdallah_elshamy>
Fri 18 Sep 2020 08:08:08 PM UTC, comment #3: 

I reviewed the functional BIST tests.  For clarity, I renamed the file from jsonencodetest.tst to jsonencode_BIST.tst and the same for jsondecodetest.tst.  I found an error decoding logical arrays which was not checked for in the BIST tests.  I will file a new report about that.  Otherwise, I added some Octave-specific tests (for 'NA' value) and used more Octave conventions (such as 80-column limit on lines where possible).  See http://hg.savannah.gnu.org/hgweb/octave/rev/b5461b1dc0ca.

I've re-titled this bug report to reflect the one remaining issue which is a need for "PrettyWriter" tests.

Rik <rik5>
Group administrator
Thu 17 Sep 2020 04:16:53 PM UTC, comment #2: 

Normally we try and keep the code and the unit tests together, but I suppose this is okay.

You're right that this probably needs some more comments.  I think what the programmer had in mind is that this script could be run under Matlab to verify that the results were equivalent in Octave.  To do so, the m-file should be left in Matlab syntax so '%' for comment character, no specialized 'end' blocks like 'endif', etc.


Rik <rik5>
Group administrator
Thu 17 Sep 2020 04:11:03 PM UTC, comment #1: 

There are some tests for jsonencode an jsondecode at this location:
https://hg.savannah.gnu.org/hgweb/octave/file/699bba597610/test/json

A note at the beginning of the files suggests that they shouldn't be moved elsewhere(?):

% Note: This script is intended to be a script-based unit test
%       for MATLAB to test compatibility.  Don't break that!


Though, I'm not really sure what that note implies.

Markus Mützel <mmuetzel>
Group administrator
Thu 17 Sep 2020 02:48:21 PM UTC, original submission:  

In a recent changeset I added input validation BIST tests, but these functions really need a set of BIST tests which verifies the various Octave data types are translated to the correct JSON data types and back again.

The tests should go at the bottom of the files jsonencode.cc and jsondecode.cc in the directory libinterp/corefcn.

Rik <rik5>
Group administrator

 

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

Attach Files:
   
   
Comment:
   

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by abdallah_elshamy (Posted a comment)
  • -email is unavailable- added by mmuetzel (Posted a comment)
  • -email is unavailable- added by rik5 (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 group members can vote.

     

    Follow 3 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2020-09-24 rik5 StatusConfirmed Fixed
        Open/ClosedOpen Closed
    2020-09-18 rik5 Summaryjsonencode/jsondecode need functional BIST tests jsonencode needs functional BIST tests for case when "PrettyWriter" is true

    Back to the top

    Powered by Savane 3.13-758e.
    Corresponding source code