Skip to content
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

[INFRA] switch to moxunit for tests #108

Merged
merged 11 commits into from
Jan 20, 2021

Conversation

Remi-Gau
Copy link
Collaborator

@Remi-Gau Remi-Gau commented Jan 13, 2021

This is an extension of the PR #100 and should probably be merged after.

This should fix #44


For reviewers:

I had to silence one test that fails with octave and not matlab.

We have some slight differences in behavior between the 2 on some queries that I would like to "fix" but I would need someone to cross check that they can reproduce them.

If they are reproducible, I would suggest to open an issue about that but to still merge that PR so we can move forward.

@Remi-Gau
Copy link
Collaborator Author

Remi-Gau commented Jan 13, 2021

EDIT: now fixed


hum...

Trying to list several directories to add to the path of the MOxUnit github action give some weird results.

    - name: MOxUnit Action
      uses: joergbrech/moxunit-action@master
      with:
        tests: tests
        src: +bids 
        ext: JSONio tests/utils      # < ------ External resources to add to the search put (excluded from coverage)
        with_coverage: true
        cover_xml_file: coverage.xml

This fails:
https://github.com/bids-standard/bids-matlab/pull/108/checks?check_run_id=1694214979

The setup phase of the action gives this:

addpath(genpath("/github/workspace/JSONio tests/utils"));

This does not seem to be the expected behavior.

Opening an issue on the github action repo.

@codecov
Copy link

codecov bot commented Jan 13, 2021

Codecov Report

❗ No coverage uploaded for pull request base (master@333c7a6). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##             master    #108   +/-   ##
========================================
  Coverage          ?   0.00%           
========================================
  Files             ?      11           
  Lines             ?     847           
  Branches          ?       0           
========================================
  Hits              ?       0           
  Misses            ?     847           
  Partials          ?       0           
Flag Coverage Δ
unittests 0.00% <0.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.


Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 333c7a6...4d6005d. Read the comment docs.

@Remi-Gau
Copy link
Collaborator Author

OK there is one assert that now fails for some unknown reason.

  pth_bids_example = get_test_data_dir();
  
  BIDS = bids.layout(fullfile(pth_bids_example, '7t_trt'));

  %   test
  mods = {'anat', 'fmap', 'func'};
  
  assert(isequal(bids.query(BIDS, 'modalities'), mods));
  assert(isequal(bids.query(BIDS, 'modalities', 'sub', '01'), mods));
  assert(isequal(bids.query(BIDS, 'modalities', 'sub', '01', 'ses', '1'), mods));
  
  % assert(isequal(bids.query(BIDS, 'modalities', 'sub', '01', 'ses', '2'), mods(2:3)));
  % this now fails on octave 4.2.2 but not on Matlab
  %
  % On octave this gives:
  % 
  % bids.query(BIDS, 'modalities', 'sub', '01', 'ses', '2')
  %
  % ans =
  % {
  %   [1,1] = anat
  %   [1,2] = fmap
  %   [1,3] = func
  % }

@Remi-Gau
Copy link
Collaborator Author

I am tempted to say we should try to put in place some sort of input validation with more meaningful error messages to make the behavior between matlab and octace more consistent.

This is the kind of output on the parts that fails

With matlab

>> BIDS = bids.layout(fullfile(pwd, 'tests', 'bids-examples', '7t_trt'));

>> bids.query(BIDS, 'subjects')
ans =
  1×22 cell array
  Columns 1 through 12
    '01'    '02'    '03'    '04'    '05'    '06'    '07'    '08'    '09'    '10'    '11'    '12'
  Columns 13 through 22
    '13'    '14'    '15'    '16'    '17'    '18'    '19'    '20'    '21'    '22'

>> bids.query(BIDS, 'sessions', 'sub', '01')
ans =
  1×2 cell array
    '1'    '2'

% the following should at leat throw a warning 
%   that subject label should be a string
>> bids.query(BIDS, 'modalities', 'sub', 1)
ans =
  0×0 empty cell array

>> bids.query(BIDS, 'modalities', 'sub', '01', 'ses', '')
ans =
  0×0 empty cell array

>> bids.query(BIDS, 'modalities', 'sub', '01', 'ses', [])
ans =
  0×0 empty cell array

>> bids.query(BIDS, 'modalities', 'sub', '01')
ans =
  1×3 cell array
    'anat'    'fmap'    'func'

% the following should at leat throw a warning
%    that session '01' does not exist with a list of the valid sessions
>> bids.query(BIDS, 'modalities', 'sub', '01', 'ses', '01')
ans =
  0×0 empty cell array

>> bids.query(BIDS, 'modalities', 'sub', '01', 'ses', '1')
ans =
  1×3 cell array
    'anat'    'fmap'    'func'

>> bids.query(BIDS, 'modalities', 'sub', '01', 'ses', '2')
ans =
  1×2 cell array
    'fmap'    'func'

With octave

>> BIDS = bids.layout(fullfile(pwd, 'tests', 'bids-examples', '7t_trt'));

>> bids.query(BIDS, 'subjects')
ans =
{
  [1,1] = 01
  [1,2] = 02
  [1,3] = 03
  [1,4] = 04
  [1,5] = 05
  [1,6] = 06
  [1,7] = 07
  [1,8] = 08
  [1,9] = 09
  [1,10] = 10
  [1,11] = 11
  [1,12] = 12
  [1,13] = 13
  [1,14] = 14
  [1,15] = 15
  [1,16] = 16
  [1,17] = 17
  [1,18] = 18
  [1,19] = 19
  [1,20] = 20
  [1,21] = 21
  [1,22] = 22
}

>> bids.query(BIDS, 'sessions', 'sub', '01')
ans =
{
  [1,1] = 1
  [1,2] = 2
}

Asking for modalities on sessions that do not exist return the same thing. (??)

>> bids.query(BIDS, 'modalities', 'sub', '01', 'ses', '02')
ans =
{
  [1,1] = anat
  [1,2] = fmap
  [1,3] = func
}

>> bids.query(BIDS, 'modalities', 'sub', '01', 'ses', '12')
ans =
{
  [1,1] = anat
  [1,2] = fmap
  [1,3] = func
}

>> bids.query(BIDS, 'modalities', 'sub', '01', 'ses', '023')
ans =
{
  [1,1] = anat
  [1,2] = fmap
  [1,3] = func
}

Both of the following return empty cell arrays in matlab.
We can have an error on the second and third one but they should be more informative.

>> bids.query(BIDS, 'modalities', 'sub', '01', 'ses', '')
ans =
{
  [1,1] = anat
  [1,2] = fmap
  [1,3] = func
}

>> bids.query(BIDS, 'modalities', 'sub', '01', 'ses', [])
error: Invalid call to lookup.  Correct usage is:

 -- IDX = lookup (TABLE, Y)
 -- IDX = lookup (TABLE, Y, OPT)
error: called from
    print_usage at line 91 column 5
    ismember at line 116 column 10
    query at line 82 column 15

>> bids.query(BIDS, 'modalities', 'sub', '01', 'ses', 2)
error: Invalid call to lookup.  Correct usage is:

 -- IDX = lookup (TABLE, Y)
 -- IDX = lookup (TABLE, Y, OPT)
error: called from
    print_usage at line 91 column 5
    ismember at line 116 column 10
    query at line 82 column 15

This is the part of the test that goes "BOINK" as session 2 only has fmap and func.

>> bids.query(BIDS, 'modalities', 'sub', '01', 'ses', '1')
ans =
{
  [1,1] = anat
  [1,2] = fmap
  [1,3] = func
}

>> bids.query(BIDS, 'modalities', 'sub', '01', 'ses', '2')
ans =
{
  [1,1] = anat
  [1,2] = fmap
  [1,3] = func
}

@Remi-Gau Remi-Gau marked this pull request as ready for review January 13, 2021 17:22
@Remi-Gau Remi-Gau changed the title [WIP] [INFRA] switch to moxunit for tests [INFRA] switch to moxunit for tests Jan 17, 2021
@Remi-Gau
Copy link
Collaborator Author

Unless I hear otherwise I will merge this in 2 days: this is mostly infrastructure related but it should make our lives easier moving forward.

@Remi-Gau Remi-Gau merged commit 7de42bf into bids-standard:master Jan 20, 2021
@Remi-Gau Remi-Gau deleted the remi-switch_to_moxunit branch January 20, 2021 10:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

using MOxUnit to run tests
1 participant