Fixes #212, Add unit test workflow by chillfig · Pull Request #219 · nasa/CF

chillfig · 2022-03-28T21:36:14Z

Changes enable_unit_test to true, changes ctest_output_on_failure to true,
Adds repo_name, changes ubuntu-latest to ubuntu-18.04, splits build-run
Copied and pasted this file from a successful run
Adds matrix method to ubuntu-18.04 and ubuntu-20.04

Checklist (Please check before submitting)

I reviewed the Contributing Guide.
I signed and emailed the appropriate Contributor License agreement to GSFC-SoftwareRelease@mail.nasa.gov and copied cfs-program@lists.nasa.gov.

Describe the contribution
Fixes #212

Testing performed
Testing performed on fork: https://github.com/chillfig/CF/actions/runs/2054990660

Expected behavior changes
Passing workflow

System(s) tested on
Ubuntu 18.04
Ubuntu 20.04

Additional context
The build release tested on Ubuntu 20.04 catches a format truncation warning in nasa/osal. #1241 documents this issue. Passing workflow requires truncation suppression, depends on nasa/cFE#2078.

Contributor Info - All information REQUIRED for consideration of pull request
Justin Figueroa, ASRC Federal

dmknutsen · 2022-03-30T16:14:26Z

On hold - Investigate why compile_options were not as expected

astrogeco · 2022-03-31T04:01:25Z

+        uses: actions/cache@v2
+        with:
+          path: /home/runner/work/${{ env.REPO_NAME }}/${{ env.REPO_NAME }}/*
+          key: build-${{ github.run_number }}-${{ matrix.buildtype }}


Add an extra item to the cache key to indicate which os you're building for, otherwise you can get a cache collision and unexpected results

astrogeco · 2022-03-31T04:01:42Z

+        uses: actions/cache@v2
+        with:
+          path: /home/runner/work/${{ env.REPO_NAME }}/${{ env.REPO_NAME }}/*
+          key: build-${{ github.run_number }}-${{ matrix.buildtype }}


Add cache key for OS

astrogeco · 2022-03-31T04:01:52Z

+        uses: actions/cache@v2
+        with:
+          path: /home/runner/work/${{ env.REPO_NAME }}/${{ env.REPO_NAME }}/*
+          key: build-${{ github.run_number }}-${{ matrix.buildtype }}          


Add cache key for OS

astrogeco · 2022-04-01T14:32:29Z

We might need to rearchitect this. If one of the builds fails the remaining builds do not run the "Run" or "Unit Test" jobs.
Even if one part of the matrix fails, the other unrelated builds should continue down the job "train"

This is actually a limmitation on github actions. I have two ideas:

Don't have separate jobs for "run" and "unit test"
Try some of the answers in this link https://stackoverflow.com/questions/65384420/how-to-make-a-github-action-matrix-element-conditional

skliper · 2022-04-04T12:57:13Z

+        os: [ubuntu-18.04, ubuntu-20.04]
+        buildtype: [debug, release]
+
+    # Set the type of machine to run on


Comment doesn't match action.

skliper · 2022-04-04T12:57:21Z

+      - name: Check Coverage
+        run: make lcov


I recommend breaking up this workflow since it seems to be doing many different things and overlapping other workflows. If the point is to just build and run CF I recommend actually checking for the CF startup message. The 2nd sed line also could be more generic (just append to the list and simply the search, probably makes more sense appending to MISSION_APP_LIST) and it's not clear why the first one adds at line 7 (seems overly specific).

Doing the entire bundle set of tests and lcov seems like overkill, suggest a separate workflow that just does the unit tests and CF coverage (with enforcement) to both simplify this workflow and avoid the potential of throwing unrelated errors.

astrogeco · 2022-04-07T17:02:10Z

  #Checks for duplicate actions. Skips push actions if there is a matching or duplicate pull-request action. 
  check-for-duplicates:
-    runs-on: ubuntu-latest
+    runs-on: ${{ matrix.os }}


checks for duplicates can run on ubuntu-latest

don't need to do the matrix

astrogeco · 2022-04-07T17:02:21Z

  #Checks for duplicate actions. Skips push actions if there is a matching or duplicate pull-request action. 
  check-for-duplicates:
-    runs-on: ubuntu-latest
+    runs-on: ${{ matrix.os }}


don't need to do the matrix

astrogeco · 2022-04-07T17:04:05Z

+      fail-fast: false
+      matrix:
+        os: [ubuntu-18.04, ubuntu-20.04]
+        buildtype: [debug, release]


This will result in 4 runs, what if we reduce that to 3 runs by running debug with ubuntu-latest and release with both 18.04 and 20.04? @skliper and @jphickey thoughts?

Might benefit from a real-time discussion. There's a handful of trades and I'd be interested in talking through it. I still don't think doing the full test and lcov are worth it at the end if the focus of this workflow is to just build and run. Building without unit tests and skipping the tests is going to save a large percentage of time.

Basically what I'm trying to say for a unit test workflow within the CF repo, just build and test CF. If you are doing a full build and run, don't do the unit test. Doing a full build with unit tests and running all the unit tests is a big investment for very little likelihood of catching an issue due to CF changes (or even related to CF at all). I'd recommend instead to add a branch at the bundle level that does all the actively maintained apps together, and use that to do the fully integrated set of tests. I have exactly such a branch in my local cFS bundle to do integrated testing. Or instead of a branch you could have a workflow that creates that specific distribution (use a script or build into makefile, it's a simple set of changes) and runs the integrated tests... probably easier to maintain that way.

Changes enable_unit_test to true, changes ctest_output_on_failure to true, Adds repo_name, Adds matrix method to ubuntu-18.04 and ubuntu-20.04 for build,run,test Adds cache key for os, moves build, run, test steps into one job, Removes redundant caches

astrogeco · 2022-05-02T03:30:01Z

Closing in favor of #235. Thanks for getting things started!

astrogeco added the CCB:Ready Ready for discussion at the Configuration Control Board (CCB) label Mar 29, 2022

astrogeco changed the title ~~Fixes #212, Adds unit test workflow~~ Fixes #212, Add unit test workflow Mar 30, 2022

astrogeco suggested changes Mar 31, 2022

View reviewed changes

chillfig force-pushed the add_unit_test branch from 74d2400 to 3b77134 Compare March 31, 2022 16:30

chillfig force-pushed the add_unit_test branch 2 times, most recently from aa21622 to 2401ec2 Compare April 1, 2022 20:58

skliper suggested changes Apr 4, 2022

View reviewed changes

chillfig self-assigned this Apr 5, 2022

astrogeco removed the CCB:Ready Ready for discussion at the Configuration Control Board (CCB) label Apr 6, 2022

chillfig force-pushed the add_unit_test branch 2 times, most recently from 2db6ba0 to 080dcde Compare April 7, 2022 15:08

astrogeco reviewed Apr 7, 2022

View reviewed changes

astrogeco suggested changes Apr 7, 2022

View reviewed changes

astrogeco reviewed Apr 7, 2022

View reviewed changes

chillfig force-pushed the add_unit_test branch from 080dcde to 3932ead Compare April 7, 2022 18:39

chillfig force-pushed the add_unit_test branch from 3932ead to 405e861 Compare April 7, 2022 19:18

astrogeco closed this May 2, 2022

chillfig deleted the add_unit_test branch May 2, 2022 13:17

skliper added the duplicate label Jul 7, 2022

Conversation

chillfig commented Mar 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dmknutsen commented Mar 30, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

astrogeco commented Apr 1, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skliper Apr 7, 2022 • 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!

astrogeco commented May 2, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

chillfig commented Mar 28, 2022 •

edited

Loading

skliper Apr 7, 2022 •

edited

Loading