-
Notifications
You must be signed in to change notification settings - Fork 12
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add option to modify files for coverage tracking in-place #5
Comments
I am not sure I understand the difference for coverage tracking in-place and the current method. Is this about changes in the path? In the current standard method, modified versions of each file are written to a temporary directory. In particular, each executable line is prefixed with a call to mocov_line_covered.m If #4 would be addressed, would this address this issue as well? |
Yes, it's due to changes in the path. Here is an example of the code which is currently being broken: Contents of REPO_DIR/package_name/utils/getPackagePath.m: function repoDir = getPackagePath()
% Get the path of this function
% This is the path to getPackagePath.m
myFunPath = mfilename('fullpath');
% Get the folder part of the path
myFunPath = fileparts(myFunPath);
% Go up a directory, twice
repoDir = fileparts(fileparts(myFunPath));
end Example test file test_something.m function test_suite = test_something()
initTestSuite;
end
function test_basic()
% Testing resources are located in the `resources` folder, within
% the `tests` folder of the main repo
resources_path = fullfile(getPackagePath(), 'tests', 'resources');
% load desired traces (as pre extracted)
fname_traces = fullfile(resources_path, 'traces.mat');
desired = load(fname_traces);
actual = something();
assertEqual(actual, desired);
end At the moment, enabling coverage means a temporary copy of getPackagePath.m is used instead of getPackagePath.m, and the temporary copy is in a different place, so its path is wrong and any test resources cannot be loaded. Closing #4 would allow me to omit But then this will not let me test the coverage of Contents of test_getPackagePath.m function test_suite = test_getPackagePath()
initTestSuite;
end
function test_contents()
% Test the path is correct by making sure the directory we get as
% the output contains certain files and folders.
expected_contents = {'package_name', 'tests', '.gitignore'};
D = dir(getPackagePath());
contents = {D.name};
message = ['Path was ' getPackagePath() ' and contents were: ' 10 ...
sprintf('%s \n', contents{:})];
assertTrue(all(ismember(expected_contents, contents)), message);
end And of course, one can envisage a scenario where there are lots of files which are path dependent, in which case omitting each of them from the coverage would be a pain (and undesirable), and running the tests on files in the same place as the originals would be an easier and better solution. |
@scottclowe it has been a while since this issue was discussed. Is this still an issue you would like to discuss further, or can I close it? |
Hi @nno, I still think it would be a useful option to have available, though it certainly should be off by default since enabling this option would result in MOcov editting the original copy of the file and then restoring a backup copy, and that can clearly lead to problems if the code is halted before completion. Of course, repositories shouldn't be contingent on their absolute path remaining the same for the code base to run correctly. (And adding the feature described in this issue would enable such bad practice.) However, copying the m-files to a different directory without copying other resources can break relative paths. So a potential alternative solution would be an option to copy all the resources, not just the m-files needing coverage tests, to a temporary directory for testing purposes? |
I think this is the issue that I hit on several times in issue #11, I think this feature would be useful. I had to change several lines of code. I am also suspecting that some of my current files are registering as not being hit by the coverage report (even though I know they are), particularly a class and an abstract class. If I debug the tests, I can see that one of my objects created actually points to my original file and not the MOcov copy for some reason, which is unknown to me as of yet. |
You mean the idea to copy all resources (files)?
This can happen in my experience with classes that are already loaded / used before MOcov is run. I ran into this myself trying to improve coverage of MOcov tests itself. I don't have a good solution for this, but suggestions / ideas are wellcome. |
DOC: Correct language in code block
This option would be useful if the location of the files is intrinsically important.
With this option, files would be copied to a temporary directory, modified to include decorators on executable lines, the tests run and coverage data collected, then the original files copied back from their temporary locations.
This is instead of the standard method which (I believe) is to write copies of the files with the decorators in them, change the MATLAB/Octave path to use these instead of the originals, run the tests and collect the coverage data, then change the path back and delete the temporary files.
In my use-case, I have a file which is used to determine the path to the package for the purpose of loading resource files contained within the package. If this file is run from a temporary directory for coverage calculation purposes, the path will be incorrect and the resources will not be loaded.
The text was updated successfully, but these errors were encountered: