-
Notifications
You must be signed in to change notification settings - Fork 184
New CI tests from applications repo #508
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
Conversation
|
@akeeste I'm getting a path error when I run these tests locally so I'm going to make some modifications to get them to work on my local machine. In general I like this concept a lot, but I'm not sold on the implementation. While I'm resolving the path issues I may make some additional changes to the test structure. Thanks for getting this started, it's a great idea! ================================================================================
Error occurred while setting up or tearing down wecSimTest.
As a result, all wecSimTest tests failed and did not run to completion.
---------
Error ID:
---------
'MATLAB:cd:NonExistentFolder'
--------------
Error Details:
--------------
Error using cd
Cannot CD to C:\Users\kmruehl\Documents\GitHub\WEC-Sim\WEC-Sim_Applications\RegularWaves (Name
is nonexistent or not a directory).
Error in run_wecSim_tests (line 128)
cd RegularWaves; printPlotRegular;
Error in wecSimTest (line 19)
run_wecSim_tests
================================================================================General thoughts/feedback:
|
|
Sorry about the path issues, I missed that when submitting. I agree, there are probably better ways to implement it. I set it up the current way to avoid adding a lot of large files to the repo (.slx, .h5, etc) for all of the application cases. I would be more straight forward but larger to keep them in the test directory. Also, my intent in picking and choosing application cases is to avoid adding excess time, and recompiling cases that have the same set-up. Similar to the above set-up, this could be more straightforward, but take much longer to run. Let me know if you want to discuss more, or want me to work on anything. |
|
@akeeste I just made some updates to the tests so that they should run on any OS... but we shall see. Let's chat about this PR to develop a path fwd. |
|
@kmruehl I made the changes to this PR that we have discussed. Now, an additional script 'setupAppFiles.m' is called in wecSimTests.m to move all necessary applications repo input files into the compilationTests directory (ignored by git). There is an additional WEC-Sim Applications path variable that is used to cd between the different directories. All paths should be using the 'fullfile' function to avoid any OS issues. Adam |
|
@kmruehl I made the discussed changes:
The compilation tests could not be wrapped in an if statement or else they did not register as a unit test. A future PR can be used to better control turning tests on/off. |
kmruehl
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks!
The purpose of this PR is to expand the continuous integration tests to include very short calls to certain Applications repo cases. The new tests are not meant to be simulation regressions, only short 'compilations'. They call applications with a very short run time so that we can check if a new commit/feature has broken a specific type of case not in the CI tests (nonhydro, drag body, GBM, B2B+CI, paraview, etc). This way we can ensure that these cases have not been broken, without running the entire simulation.
My current method of doing this is to read in a case's original input file, and append new parameters to the end of it. For example:
simu.explorer = 'off';
simu.startTime = 0;
simu.rampTime = 2;
simu.endTime = 4;
simu.dt = 0.1;
Then wecSim is called for that case. Once run successfully, the output is removed, and the original input file is rewritten to the applications case directory.
This does require that the WEC-Sim_Applications repo be in a directory on the same level as WEC-Sim and up to date. Currently it would overwrite any that is presently there.
Currently it increases the time taken to run the tests by about 20% (from ~5 minutes to ~6 minutes on my laptop). The majority of this is the case set-up, lowering simu.endTime further has a marginal impact.
Right now the applications used are:
runB2BCase4; % covers b2b, regularCIC wave, ode4
runB2BCase6; % covers b2b + SS, regularCIC, ode4
runDecayME; % covers nowaveCIC and morison element
runGBM; % covers gbm, ode45, regular wave
runMCR; % covers mcr with spectrum import and mcr casefile import
runMooring; % covers mooring matrix
runNonHydro; % covers nonhydro body
runParaview; % covers nonlinear hydro, paraview, simulink accelerator
@kmruehl We discussed this a little bit previously, I'm making a PR so that it can be reviewed. Let me know your thoughts. I think it will be helpful in avoiding bugs like some that have come up recently.
I'm not set on the way everything is called. For example, I think it might be cleaner to keep copies of the relevant input files in the tests directory and copy the new/old ones back and forth. Alternatively we can keep simulink files in the tests directory too so that someone's Applications repo isn't overwritten, but that would increase the repo size.