gnu-octave/pkg-jupyter-notebook

Handle the "ans" variable in the context

Abdallah-Elshamy opened this issue ยท 6 comments

The ans variable is currently supported if it is printed in the output. We want to support if it is assigned but the new value is not printed

@siko1056 , I pushed a commit that solves this by adding a new statement that prints the value of ans at the end of the cell then remove the printed value from the output.

Please, check it and if you have any comments let me know.
If all is okay, we can release the first version.

Sorry for the late reply and thank you for your changes @Abdallah-Elshamy ๐Ÿ™‚

While running the test suite, test JupyterNotebook, I receive some error messages. Looking closer at the code, I often see this constructs:

%! assert (n.notebook.cells{5}.outputs{1}.text,
%! {"ans = \nerror: 'a' undefined near line 1 column 2"});

This seems a little fragile to test for the exact shape of an error message ๐Ÿ˜“ In every Octave release a whitespace for example might be introduced and all our BIST are failing ๐Ÿ˜…

Therefore, I would prefer to not test for such error strings and remove those tests from the BISTs. Rather checking if the working output is correct, seems more "safe" to me. Although here whitespace might change over time, let alone format compact/losse/long/short might ruin such checks...

Even though this might be less satisfying, maybe it is safer if we reduce the whole BIST to plain functional tests, i.e., no errors occur while running your JupyterNotebook class functions, rather than checking string outputs byte-wise.

If you want to release the package, you can start any time on Octave packages ๐Ÿ˜‰ Asking for some testers of your freshly released package on Octave Discourse might also bring up some unforeseen bug reports.

I think, once we got the BISTs in shape (next week) we push your class and BIST to core Octave.

Therefore, I would prefer to not test for such error strings and remove those tests from the BISTs. Rather checking if the working output is correct, seems more "safe" to me. Although here whitespace might change over time, let alone format compact/losse/long/short might ruin such checks...

In the past couple of days, I was looking for a more reliable way to test for those changes as they test a core feature of the package but I couldn't find any, unfortunately ๐Ÿ˜…. I also agree with you that they are very fragile and it is better to remove those tests.

Even though this might be less satisfying, maybe it is safer if we reduce the whole BIST to plain functional tests, i.e., no errors occur while running your JupyterNotebook class functions, rather than checking string outputs byte-wise.

The tests for embedding images should be perfectly fine as they are embedded by the package and they are not run by Octave. Markdown cells are also treated in the same way. I think it is better to leave them as they are and reduce only the tests for embedding textual output to test for the structure of the notebook (the correct keys exist in any struct in the outputs). What do you think :quri?

I pushed a commit modifying the tests. Please, check it and if you think that the tests require more adjustments, let me know.

If you want to release the package, you can start any time on Octave packages wink Asking for some testers of your freshly released package on Octave Discourse might also bring up some unforeseen bug reports.

If you want to release the package, you can start any time on Octave packages wink Asking for some testers of your freshly released package on Octave Discourse might also bring up some unforeseen bug reports.

I think, once we got the BISTs in shape (next week) we push your class and BIST to core Octave.

That sounds great ๐Ÿ’ช. Once we fix the BISTs issues we can do that.

@siko1056 , Any updates?

Sorry for letting you wait. I wrote an email with further suggestions.

As I have hijacked this report with the testing issue, let us return to the original issue with the ans variable, which is actually solved by you two weeks ago. Great work ๐Ÿ™‚ ๐Ÿ‘