restructure CI tests into class-based unit tests #620

akeeste · 2021-06-21T18:54:58Z

This PR restructures the CI tests into class-based continuous integration tests. Right now there are classes for each:

regression tests
compilation tests
BEMIO
rotation tests
run from Simulink tests

Each class has specific setup, teardown and test methods. I also added tests for the BEMIO functions to ensure they are working correctly. This should make it easier to properly skip long tests (irregular Passive Yaw, MCR application, etc) by marking them "incomplete" instead of "failed".

I still need to work on improving the test runtime. Creating a draft PR so @H0R5E can review.

akeeste · 2021-06-22T22:57:48Z

In a few local trials I have the following approximate breakdown of runtime per test class, including the contributions of the longest individual tests. In total, the tests take about ~11 minutes when run on my laptop.

BEMIO tests: 10%
Compilation (application) tests: 51%
- B2B case 4: 18%
- MCR case: 8.5%
- mooring case: 7.5%
Regression tests: 36%
- regular cases: 5%
- irregular cases: 4%
- passive yaw cases: 27%
Rotation tests: 0.03%
Run from Simulink tests: 3%

If the application repo tests are removed as suggested in issue #619, in addition to skipping the passive yaw cases, this decreases the test runtime by ~80%. The integration tests can be gradually broken down into unit tests over time to prevent duplicate tests. This will improve runtime even further in the future.

H0R5E · 2021-06-23T12:13:59Z

See akeeste#7

Use test suites to control execution

akeeste · 2021-07-06T16:16:21Z

A few additions since this PR has been opened:

removed applications tested from the WEC-Sim repo. Applications Repo PR 7 adds these tests to that repo.
moved the passive yaw regression tests to a separate class so that they can be turned on/off easily. By default they are skipped to decrease run time
Changed how the tests are called in wecSimTest. Now this function uses the run(suite) command instead of testRunner.run(suite) so that appropriate debugging information is collected in the test results.

As indicated in the above discussion, removing the applications and passive yaw tests significantly decreases run time. Now the tests take ~2 minutes locally, only 15% of the previous run time.

H0R5E · 2021-07-06T17:02:16Z

Adam, how much time is saved by not running the passive yaw regression test? My concern is that if they are not run by default, then they will never be run at all.

I would also consider replacing the test runner. If we are going to collect coverage info (which I think we should soon) we need to use a plugin on a test runner:

https://uk.mathworks.com/help/matlab/ref/matlab.unittest.plugins.codecoverageplugin-class.html

What is the difference in output between using run and using the test runner?

akeeste · 2021-07-06T17:54:57Z

@H0R5E
That is a fair point, most users might not remember to turn on passive yaw when appropriate. Maybe we just remove the passive yaw regression altogether? The functionality will be covered in the Applications repository, though without a comparison to the previous result. After the compilation/Application tests are removed, the passive yaw tests take up ~50% of the runtime.

My problem with the test runner is that it currently does not give the same diagnostic information as the run command (in R2021a). For example, right now the first BEMIO test fails (the fix is already in dev).

When using the testRunner, results(1).Details is empty. Whereas the run(suites) command gives useful debugging information in Details.DiagnosticRecord such as Exception, Stack, Scope, Report that provide details on where the error occurred and the error message received.

This almost seems like a Matlab bug, I don't know why the testRunner would give less information than the standard run command. If the testRunner will be required for code coverage information, I can look again at how to get around this issue. However until code coverage is implemented, I think the run command is more insightful.

H0R5E · 2021-07-06T18:09:10Z

I think I might have been calling the runner incorrectly. Can you try the method here: https://uk.mathworks.com/help/matlab/ref/matlab.unittest.testrunner-class.html#mw_8602d5ca-8748-4425-8fa1-9c2fdaef84e3 I.e. runner = TestRunner.withTextOutput; result = run(runner,suite); T

…

On Tue, Jul 6, 2021, 18:55 Adam Keester ***@***.***> wrote: @H0R5E <https://github.com/H0R5E> That is a fair point, most users might not remember to turn on passive yaw when appropriate. Maybe we just remove the passive yaw regression altogether? The functionality will be covered in the Applications repository, though without a comparison to the previous result. My problem with the test runner is that it currently does not give the same diagnostic information as the run command (in R2021a). For example, right now the first BEMIO test fails (the fix is already in dev). When using the testRunner, results(1).Details is empty. Whereas the run(suites) command gives useful debugging information in Details.DiagnosticRecord such as Exception, Stack, Scope, Report that provide details on where the error occurred and the error message received. This almost seems like a Matlab bug, I don't know why the testRunner would give less information than the standard run command. If the testRunner will be required for code coverage information, I can look again at how to get around this issue. However until code coverage is implemented, I think the run command is more insightful. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#620 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABG2KF6UZ65A3KHGMSLUZ63TWM7PXANCNFSM47CEPKUA> .

H0R5E · 2021-07-06T18:29:55Z

It's weird though, they should be equivalent ways of calling the same thing. Do you mind if I take a look at this tomorrow morning? Mat

…

On Tue, Jul 6, 2021, 19:08 Mathew Topper ***@***.***> wrote: I think I might have been calling the runner incorrectly. Can you try the method here: https://uk.mathworks.com/help/matlab/ref/matlab.unittest.testrunner-class.html#mw_8602d5ca-8748-4425-8fa1-9c2fdaef84e3 I.e. runner = TestRunner.withTextOutput; result = run(runner,suite); T On Tue, Jul 6, 2021, 18:55 Adam Keester ***@***.***> wrote: > @H0R5E <https://github.com/H0R5E> > That is a fair point, most users might not remember to turn on passive > yaw when appropriate. Maybe we just remove the passive yaw regression > altogether? The functionality will be covered in the Applications > repository, though without a comparison to the previous result. > > My problem with the test runner is that it currently does not give the > same diagnostic information as the run command (in R2021a). For example, > right now the first BEMIO test fails (the fix is already in dev). > > When using the testRunner, results(1).Details is empty. Whereas the > run(suites) command gives useful debugging information in > Details.DiagnosticRecord such as Exception, Stack, Scope, Report that > provide details on where the error occurred and the error message received. > > This almost seems like a Matlab bug, I don't know why the testRunner > would give less information than the standard run command. If the > testRunner will be required for code coverage information, I can look again > at how to get around this issue. However until code coverage is > implemented, I think the run command is more insightful. > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#620 (comment)>, or > unsubscribe > <https://github.com/notifications/unsubscribe-auth/ABG2KF6UZ65A3KHGMSLUZ63TWM7PXANCNFSM47CEPKUA> > . >

akeeste · 2021-07-06T19:02:21Z

Hi Mat,

Yes I'll take a look at this today and can discuss with you over the next few days.

akeeste · 2021-07-06T22:13:54Z

@H0R5E

I looked at the test plugins more today and discovered the one that seems to be recording diagnostic information in the testResult struct. I tested the following TestRunner options that contain the respective plugins:

TestRunner.withNoPlugins;      % no plugins
TestRunner.withDefaultPlugins; % ConciseProgressPlugin, DiagnosticsOutputPlugin, DiagnosticsRecordingPlugin
TestRunner.withTextOutput;     % TestRunProgressPlugin, DiagnosticsOutputPlugin

DiagnosticsRecordingPlugin is the option that adds the diagnostic information to the testResult object. This is included as the default plugin when run(suites) is used and why we were seeing different output. The testRunner class also has a note that the 'withDefaultPlugins' option will change across versions.

Given these items, I opted for the following setup, which will contain TestRunProgressPlugin, DiagnosticsOutputPlugin, DiagnosticsRecordingPlugin and CodeCoveragePlugin. This results in sufficient debugging information and allows code coverage in the future.

runner = TestRunner.withTextOutput;
runner.addPlugin(DiagnosticsRecordingPlugin);
runner.addPlugin(CodeCoveragePlugin.forFolder('./source'));

.

H0R5E · 2021-07-07T11:30:55Z

@akeeste, good work. It's kind of weird that the DiagnosticsRecordingPlugin gets added in certain situations but not others. I would say that is probably a bug.

Do you want me to review the PR?

akeeste · 2021-07-07T14:23:03Z

@H0R5E Yes that would be great. There should not be much changed since you pushed to this branch.

Last note--I am aware of one BEMIO test currently failing. The dev branch already contains the fix for this and the tests should all pass once the PR is merged.

H0R5E

Hi @akeeste, overall I think this is a really good update to the testing framework. I've got a few minor style changes that I suggested in comments, but I also think there are three "blockers" to merging this right now. These are:

The test suite should not fail just because of the combine_bem test. I've added a solution to this in akeeste#8
There is no documentation added to describe how to use the test suite. There should be something, even if its just a docstring for the wecSimTest function.
I don't think we should skip the passive yaw tests by default. It was 200 seconds without and 260 seconds with on my machine and I think once you're in the minutes an extra one won't hurt. Explaining to users how to turn off the tests (in the doctring or whatever) is a better approach.

I'm happy to help out with any of these changes, so just give me a shout.

Mat

EDIT: It's best to look at my comments in the Files changed tab, rather than below.

source/functions/wecSimMCR.m

source/functions/wecSimStopFunction.m

tests/bemioTest.m

H0R5E · 2021-07-08T11:00:57Z

tests/bemioTest.m

+            hydro(end).body = {'cylinder_nemoh'};
+            hydro = Read_WAMIT(hydro,'..\..\WAMIT\Cylinder\cyl.out',[]);
+            hydro(end).body = {'cylinder_wamit'};
+            hydro = Combine_BEM(hydro);


It is better to skip this test as a known error, rather than leave it as a failure. The automated CI is always going to fail if we leave it like this. See akeeste#8 for the approach to skip this test based on a known failure

I agree the test should be skipped if the fix is not already present. However since this PR is only merging into dev, the fix will be present and the test will pass as soon as the PR is merged. To simplify this I pulled dev into this branch so that all tests are passing before the merge.

Oh, I see. Shouldn't we be putting this into master as well?

I assumed this wouldn't happen until the entire dev branch is merged into master and a new release is tagged. I believe v4.3 was planned for the near future.

tests/rotationTest.m

tests/runFromSimTest.m

wecSimTest.m

…ructure

H0R5E · 2021-07-13T13:29:31Z

@akeeste, @kmruehl is there a reason that the mat files are not stored using LFS? They are super big, huh, so would LFS not be a better approach?

H0R5E · 2021-07-13T14:02:59Z

Offered an improvement to the docstring in akeeste#9

Improve docstring of wecSimTest.m

H0R5E

Looks great. Please go ahead and merge it (I would recommend a squash and merge).

akeeste added 7 commits June 3, 2021 09:20

restructure CI tests into class-based unit tests

85c3ecf

add BEMIO test class

e3a393f

update compilation and regression tests

d2f06bf

merge with dev tests

3bfd860

run from simulink test class

b67a52c

rotation test class

327992e

decrease BEMIO test time

49b61d2

akeeste added Tests/CI related WEC-Sim tests or Continuous Integration Feature new feature request labels Jun 21, 2021

akeeste added 2 commits June 21, 2021 17:21

add file closing to wecSim

56c18b5

clean up class definitions

9696774

akeeste mentioned this pull request Jun 22, 2021

[Developer Issue] WEC-Sim Testing Master Plan (Proposal) #619

Closed

10 tasks

Use TestSuites to control test execution and do less path gymnastics

380eee1

Merge pull request #7 from H0R5E/test_suites

10c1242

Use test suites to control execution

H0R5E mentioned this pull request Jun 29, 2021

Add tests for all applications and limit mechanics explorer opening. WEC-Sim/WEC-Sim_Applications#7

Merged

8 tasks

akeeste added 2 commits July 2, 2021 11:51

remove compilation tests

64d7112

move passive yaw tests to separate class

f74214c

akeeste marked this pull request as ready for review July 6, 2021 16:12

update test runner plugins

a6480c5

akeeste requested a review from H0R5E July 7, 2021 14:11

H0R5E suggested changes Jul 8, 2021

View reviewed changes

akeeste added 3 commits July 9, 2021 08:57

clean up tests

824f882

Merge branch 'dev' of https://github.com/WEC-Sim/WEC-Sim into ci_rest…

8531de7

…ructure

cleanup

f0d484e

Improve docstring of wecSimTest.m

93132ce

Merge pull request #9 from H0R5E/improved_docstring

d015afe

Improve docstring of wecSimTest.m

H0R5E approved these changes Jul 14, 2021

View reviewed changes

akeeste merged commit 8553761 into WEC-Sim:dev Jul 14, 2021

akeeste deleted the ci_restructure branch July 14, 2021 14:18

restructure CI tests into class-based unit tests #620

restructure CI tests into class-based unit tests #620

Uh oh!

Conversation

akeeste commented Jun 21, 2021

Uh oh!

akeeste commented Jun 22, 2021

Uh oh!

H0R5E commented Jun 23, 2021

Uh oh!

akeeste commented Jul 6, 2021

Uh oh!

H0R5E commented Jul 6, 2021

Uh oh!

akeeste commented Jul 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

H0R5E commented Jul 6, 2021 via email

Uh oh!

H0R5E commented Jul 6, 2021 via email

Uh oh!

akeeste commented Jul 6, 2021

Uh oh!

akeeste commented Jul 6, 2021

Uh oh!

H0R5E commented Jul 7, 2021

Uh oh!

akeeste commented Jul 7, 2021

Uh oh!

H0R5E left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

H0R5E Jul 8, 2021

Choose a reason for hiding this comment

Uh oh!

akeeste Jul 9, 2021

Choose a reason for hiding this comment

Uh oh!

H0R5E Jul 9, 2021

Choose a reason for hiding this comment

Uh oh!

akeeste Jul 9, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

H0R5E commented Jul 13, 2021

Uh oh!

H0R5E commented Jul 13, 2021

Uh oh!

H0R5E left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

akeeste commented Jul 6, 2021 •

edited

Loading

H0R5E left a comment •

edited

Loading