Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
Already on GitHub? Sign in to your account
ENH: Add minimal classdef support for MOcovMFile #9
Conversation
|
Thanks, great work! This looks very good to me. |
| + initTestSuite; | ||
| +end | ||
| + | ||
| +function fullname = tempfile(filename, contents) |
nno
Dec 20, 2016
Contributor
Can you change the name to something that indicates that contents is written, for example write_to_tempfile?
abravalheri
Dec 20, 2016
•
Sure!
I was wondering about creating a 'support' folder inside 'tests', to this and other methods that can be shared. What do you thing?
| + | ||
| +function fullname = tempfile(filename, contents) | ||
| + tempfolder = fullfile(tempdir, 'mocov_fixtures'); | ||
| + [~, ~, ~] = mkdir(tempfolder); |
nno
Dec 20, 2016
Contributor
- it seems unnecessary to have output parameters here.
mkdir(tempfolder);should be sufficient - please do not use
~for unused output parameters, as older versions of Matlab do not support it. Most of the time I useunusedas variable name instead.
abravalheri
Dec 20, 2016
If I am not wrong, Matlab issues a warning if mkdir tries to create an already existent, unless all the 3 return values are captured.
Can I opt by the unused suggestion?
| +end | ||
| + | ||
| +function test_classdef_line_not_executable | ||
| + mfile = MOcovMFile(create_classdef); |
nno
Dec 20, 2016
Contributor
It seems that after the test, the file and directory that were created are not deleted afterwards. Can you please add the required statements, preferably using onCleanup?
For example see https://github.com/MOxUnit/MOxUnit/blob/master/tests/test_moxunit_util_elem2str.m, function remove_path_directory minus the rmpath statement.
Alternatively it may be sufficient to just create a temporary file with tempname, without a directory, and delete the file afterwards.
abravalheri
Dec 20, 2016
The point about having a temporary folder is to be confident about using addpath (I don't know what kind of mfiles exist under the temp dir, and maybe them can shadow some other function)...
I guess a way to avoid it is explicitly requiring the folder to be specified in the filename (is this your suggestion in other words?).
| + method_opening = [8, 13]; | ||
| + | ||
| + for l = method_opening | ||
| + assert(~executable_lines(l), ... |
nno
Dec 20, 2016
Contributor
What if at some point lines in create_classdef were deleted or added? Then line numbers would not match anymore...
One possibility is to include a 'test of the test', for example, say we would add a method called assert_line_contains, then here we could include within the for loop:
assert_line_contains(lines{i},'methods');.
abravalheri
Dec 20, 2016
When I was writing the tests, I was also uncomfortable with the hard coded approach, but without ideas about how to make it simple.
This suggestion seems to have a perfect balance! Thank you.
| + method_lines = [10, 15]; | ||
| + | ||
| + for l = method_lines | ||
| + assert(executable_lines(l), ... |
| + properties_lines = [2:4, 5:7]; | ||
| + | ||
| + for l = properties_lines | ||
| + assert(~executable_lines(l), ... |
abravalheri
Dec 20, 2016
With line ranges do you think 'assert(ing)_line_contains' just the first and last lines are enough?
| @@ -0,0 +1,68 @@ | ||
| +function test_suite = test_write_lines_with_prefix_generate_valid_classdef_files | ||
| + initTestSuite; |
nno
Dec 20, 2016
Contributor
Can we merge this file with test_MOcovMFile_recognizes_classdef_syntax.m? There seems to be code duplication in the tempfile and create_classdef functions, and it seems that the remaining test_generate_valid_file function can just be moved.
| +end | ||
| + | ||
| +function fullname = tempfile(filename, contents) | ||
| + tempfolder = fullfile(tempdir, 'mocov_fixtures'); |
nno
Dec 20, 2016
Contributor
Please see comments for file test_MOcovMFile_recognizes_classdef_syntax.m.
|
@abravalheri I would also be happy to implement the suggested changes myself - just please let me know. |
abravalheri
reviewed
Dec 20, 2016
@nno thank you for the review.
Some consideration follows and I will submit another proposal for review ASAP.
| + initTestSuite; | ||
| +end | ||
| + | ||
| +function fullname = tempfile(filename, contents) |
abravalheri
Dec 20, 2016
•
Sure!
I was wondering about creating a 'support' folder inside 'tests', to this and other methods that can be shared. What do you thing?
| + | ||
| +function fullname = tempfile(filename, contents) | ||
| + tempfolder = fullfile(tempdir, 'mocov_fixtures'); | ||
| + [~, ~, ~] = mkdir(tempfolder); |
abravalheri
Dec 20, 2016
If I am not wrong, Matlab issues a warning if mkdir tries to create an already existent, unless all the 3 return values are captured.
Can I opt by the unused suggestion?
| +end | ||
| + | ||
| +function test_classdef_line_not_executable | ||
| + mfile = MOcovMFile(create_classdef); |
abravalheri
Dec 20, 2016
The point about having a temporary folder is to be confident about using addpath (I don't know what kind of mfiles exist under the temp dir, and maybe them can shadow some other function)...
I guess a way to avoid it is explicitly requiring the folder to be specified in the filename (is this your suggestion in other words?).
| + method_opening = [8, 13]; | ||
| + | ||
| + for l = method_opening | ||
| + assert(~executable_lines(l), ... |
abravalheri
Dec 20, 2016
When I was writing the tests, I was also uncomfortable with the hard coded approach, but without ideas about how to make it simple.
This suggestion seems to have a perfect balance! Thank you.
| + properties_lines = [2:4, 5:7]; | ||
| + | ||
| + for l = properties_lines | ||
| + assert(~executable_lines(l), ... |
abravalheri
Dec 20, 2016
With line ranges do you think 'assert(ing)_line_contains' just the first and last lines are enough?
added some commits
Dec 20, 2016
abravalheri
pushed a commit
to abravalheri/MOcov
that referenced
this pull request
Dec 20, 2016
|
|
+ |
f3dd7e3
|
added some commits
Dec 20, 2016
|
@abravalheri thanks for all the changes, it has really improved. A few minor questions:
|
|
@abravalheri I would also be happy to implement these minor suggestions myself and then merge. Please let me know your preference. |
abravalheri
commented
Dec 24, 2016
|
@nno Sorry, for the delay. I was planning to finish this pull request by following your suggestions. I just have a doubt about the second one: After my last commit, the only place left a tempfolder was used is to store the decorated file, since it has the same name that the original file (already stored in the tempdir) and needs to run in order to automatically verify if its syntax is correct (unfortunately octave has no linter, and I think this test is important because is the only that directly assert all the things work as expected). There is a workaround for it: just create the original file with an arbitrary name that do not corresponds to the class name (since it does not run)... But this does not exactly corresponds to the reality (do it?), and seems to me a little bit hacky. Well, tests (besides automatic verification) are also supposed to serve as documentation, and this workaround may confuse unadvised readers ... |
abravalheri
commented
Dec 24, 2016
•
|
Right now, I was trying to remove the I don't know the internals of MOxUnit (does it use the $ ls tests
test_mocov_line_covered.m test_get_absolute_path.m create_tempfolder.m
test_mocov_is_absolute_path.m test_MOcovMFile_recognizes_classdef_syntax.m create_tempfile.m
test_mocov_get_relative_path.m ensure_path_in_tempdir.m assertStringContains.m
$ make test
make test-matlab
make[1]: Entering directory `/home/vagrant/drafts/MOcov'
matlab binary could not be found, skipping
make[1]: Leaving directory `/home/vagrant/drafts/MOcov'
make test-octave
make[1]: Entering directory `/home/vagrant/drafts/MOcov'
if [ -n "/usr/bin/octave" ]; then \
/usr/bin/octave --no-gui --quiet --eval "orig_dir=pwd();cd('/home/vagrant/drafts/MOcov/MOcov');addpath(pwd);cd(orig_dir);if(isempty($
hich('moxunit_runtests'))),error('MOxUnit is required; see https://github.com/MOxUnit/MOxUnit');end;success=moxunit_runtests('/home/vagrant/drafts/M$
cov/tests');exit(~success);"; \
else \
echo "octave binary could not be found, skipping"; \
fi;
suite: 9 tests
FFFFF....
--------------------------------------------------
failure: 'create_tempfile' undefined near line 10 column 16
test_MOcovMFile_recognizes_classdef_syntax>create_classdef:10 (/home/vagrant/drafts/MOcov/tests/test_MOcovMFile_recognizes_classdef_syntax.m)
test_MOcovMFile_recognizes_classdef_syntax>test_classdef_line_not_executable:35 (/home/vagrant/drafts/MOcov/tests/test_MOcovMFile_recognizes_classdef_syntax.m)
failure: 'create_tempfile' undefined near line 10 column 16
test_MOcovMFile_recognizes_classdef_syntax>create_classdef:10 (/home/vagrant/drafts/MOcov/tests/test_MOcovMFile_recognizes_classdef_syntax.m)
test_MOcovMFile_recognizes_classdef_syntax>test_methods_opening_section_not_executable:50 (/home/vagrant/drafts/MOcov/tests/test_MOcovMFile_recognizes_classdef_syntax.m)
failure: 'create_tempfile' undefined near line 10 column 16
test_MOcovMFile_recognizes_classdef_syntax>create_classdef:10 (/home/vagrant/drafts/MOcov/tests/test_MOcovMFile_recognizes_classdef_syntax.m)
test_MOcovMFile_recognizes_classdef_syntax>test_method_body_executable:68 (/home/vagrant/drafts/MOcov/tests/test_MOcovMFile_recognizes_classdef_syntax.m)
failure: 'create_tempfile' undefined near line 10 column 16
test_MOcovMFile_recognizes_classdef_syntax>create_classdef:10 (/home/vagrant/drafts/MOcov/tests/test_MOcovMFile_recognizes_classdef_syntax.m)
test_MOcovMFile_recognizes_classdef_syntax>test_properties_line_not_executable:86 (/home/vagrant/drafts/MOcov/tests/test_MOcovMFile_recognizes_classdef_syntax.m)
failure: 'create_tempfile' undefined near line 10 column 16
test_MOcovMFile_recognizes_classdef_syntax>create_classdef:10 (/home/vagrant/drafts/MOcov/tests/test_MOcovMFile_recognizes_classdef_syntax.m)
test_MOcovMFile_recognizes_classdef_syntax>test_generate_valid_file:117 (/home/vagrant/drafts/MOcov/tests/test_MOcovMFile_recognizes_classdef_syntax.m)
--------------------------------------------------
FAILED (failure=5)
make[1]: *** [test-octave] Error 1
make[1]: Leaving directory `/home/vagrant/drafts/MOcov'
make: *** [test] Error 2(According to my experience with another Octave project, Octave seems to implement the (Tested on Ubuntu 14.04, Octave 4.0.2 x86_64-pc-linux-gnu) |
|
@abravalheri thanks again for all your work on this.
|
abravalheri
commented
Dec 24, 2016
|
@nno I have made some changes following the direction you pointed (and also added a comment to clarify the 'original X decorated file' name sharing dilemma). Thank you for the revision. |
|
@abravalheri thanks for the final changes, it looks great. Il will merge this PR now. |
abravalheri commentedDec 18, 2016
Avoid prepending
mocov_line_coveredto:classdef;propertiesandmethodssection opening statements;propertiessection, since a limited subset of the M-file syntax is allowed.This change tries to solve issue #8 .