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.

Sign up for GitHub

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

Jump to bottom

Fix issue 286 - Code tidying #287

Closed
wants to merge 18 commits into from
Closed

Fix issue 286 - Code tidying #287

wants to merge 18 commits into from

Conversation

teddychao
Copy link
Contributor

@teddychao teddychao commented Jul 19, 2021 •
edited
Loading

Fixes #286 .

Changes proposed in this pull request:

  • All the test files which are generated at the beginning of each test function are thereafter cleared once the test function is finished. This is deigned to save space because GitHub Actions has limited storage under the current plan.
  • The temporary data files that are created by the sub functions are cleared within the corresponding sub function, this is because the clearance for them at the end of the global test function is too complicated.

Details are provided below

  • pspm_load_data_test
    • fn2 is now a global constant within this test function.
    • fn is renamed as fn1.
    • fn and fn2 are cleared when this function is finished.
  • pspm_split_sessions_test
    • fn (in one_datafile) is now a global constant within this test function.
  • pspm_trim_test
    • fn2 is now a global constant within this test function.
    • fn1 and fn2 are cleared when this function is finished.

Miscellaneous

  • Some old code notes are removed in this version, like

% $Id: pspm_XXX_test.m 646 YYYY-MM-DD HH:MM:SSZ esrefo $

@teddychao teddychao marked this pull request as draft July 19, 2021 17:13
@teddychao teddychao self-assigned this Jul 19, 2021
teddychao and others added 7 commits July 19, 2021 20:38
@teddychao
See notes
ac078dc 
Update find_free_fn because the number was too close to the original file name
Update metadata for the three test functions
Update the test file name by using the corresponding test function name
Trim: some fixed values were generalised to be global constant
@teddychao
Revert the location of some test functions
51316e5
@teddychao
Update pspm_trim_test.m
434d261
@teddychao
Update pspm_trim_test.m
ab94a62
Merge branch 'develop' into Issue-286
b2f5ed3
@teddychao
Incorrect test file load variable is fixed
8a78be4
@teddychao
Create pspm_test_temp.m
d3dea5f
@teddychao teddychao added In Progress Currently being worked on Low Priority Feature request with not high priority and removed In Progress Currently being worked on labels Sep 3, 2021
@teddychao teddychao removed the request for review from dominikbach November 13, 2021 19:04
@teddychao teddychao marked this pull request as ready for review November 13, 2021 19:04
@teddychao teddychao marked this pull request as draft November 13, 2021 19:11
@teddychao teddychao changed the title Fix issue 286 Fix issue 286 - Code tidying Dec 12, 2021
jbrochar
jbrochar approved these changes May 11, 2022
View reviewed changes
Copy link

@jbrochar jbrochar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just had light and skippable comments

test/pspm_split_sessions_test.m

methods (Test)
function invalid_input(this)
this.verifyWarning(@()pspm_split_sessions(), 'ID:invalid_input');
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Weird end of line character here :)

Copy link
Contributor Author

@teddychao teddychao May 31, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this issue has been resolved in a recent branch where we converted every file into UTF-8 and LF.

test/pspm_test_temp.m
% TestSuite.fromClass(?pspm_pupil_pp_test), ...
% TestSuite.fromClass(?set_blinks_saccades_to_nan_test), ...
% TestSuite.fromClass(?pspm_blink_saccade_filt_test), ...
% TestSuite.fromClass(?pspm_convert_gaze_distance_test), ...
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is intended to stay commented, or is it a temporary fix?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jbrochar Hi jules, this is just used for testing the target function by me, I need to add it to .gitignore.

@teddychao
Copy link
Contributor Author

This issue is also mentioned in the recent branch of loading help text for the GUI.

@teddychao teddychao closed this May 31, 2022
@teddychao teddychao deleted the Issue-286 branch May 31, 2022 10:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jbrochar jbrochar jbrochar approved these changes

Assignees

@teddychao teddychao

Labels
Low Priority Feature request with not high priority
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Test function regularisation
3 participants
@teddychao @jbrochar @dominikbach