Skip to content

Conversation

@akeeste
Copy link
Contributor

@akeeste akeeste commented Feb 4, 2021

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.

@akeeste akeeste requested a review from kmruehl February 4, 2021 23:51
@akeeste akeeste self-assigned this Feb 4, 2021
@akeeste akeeste added Feature new feature request Tests/CI related WEC-Sim tests or Continuous Integration WEC-Sim Application related to a WEC-Sim Application case (https://github.com/WEC-Sim/WEC-Sim_Applications) labels Feb 4, 2021
@kmruehl
Copy link
Collaborator

kmruehl commented Mar 2, 2021

@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:

  • It's a little counter-intuitive that only a few of the application cases are being tested
  • Perhaps we should remove passive yaw from the regression tests and move it to the compilation tests since they take so long to run
  • Maybe we should have an option to run the compilation tests as regression tests if people are working on something that is likely to impact a specific feature (e.g. ME or MCR)
  • Changing directories is very tricky and causes a lot of issues, especially if you select different options (e.g. no plotting, no reg wave regression tests), or don't have the exact same path structure. We should think about how to make this a little more robust, perhaps by providing the option to specify local directories?
  • Maybe some of these path issues can be resolved by copying the application case into the compilation tests directory and running them in the compilation tests directory. This also resolves the issue of not overwriting existing results for application cases.

@akeeste
Copy link
Contributor Author

akeeste commented Mar 2, 2021

@kmruehl

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.

@kmruehl
Copy link
Collaborator

kmruehl commented Mar 2, 2021

@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 kmruehl closed this Mar 3, 2021
@kmruehl kmruehl deleted the branch WEC-Sim:dev March 3, 2021 20:58
@kmruehl kmruehl reopened this Mar 3, 2021
@akeeste
Copy link
Contributor Author

akeeste commented Mar 23, 2021

@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

@akeeste
Copy link
Contributor Author

akeeste commented Apr 26, 2021

@kmruehl I made the discussed changes:

  • removed incorrect compilation tests assert statements
  • combined run_wecSimTest.m into wecSimTest.m

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.

Copy link
Collaborator

@kmruehl kmruehl left a comment

Choose a reason for hiding this comment

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

thanks!

@kmruehl kmruehl merged commit 1e715cb into WEC-Sim:dev May 19, 2021
@akeeste akeeste deleted the new_tests branch May 19, 2021 22:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Feature new feature request Tests/CI related WEC-Sim tests or Continuous Integration WEC-Sim Application related to a WEC-Sim Application case (https://github.com/WEC-Sim/WEC-Sim_Applications)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants