Fix #1690, Tests for time conversion api #1707

paulober · 2021-07-25T18:54:33Z

Describe the contribution

Fixes Add functional tests for cFE Time Conversion APIs #1690 (New unit-tests for the Time Conversion API like described in this issue)

Expected behavior changes

No impact on behavior

Additional context
Hi, I'm new to this repository and tried my hand at a "good first issue". I would be very grateful if someone who knows about this repo can have a quick look and tell me if my first commit for this issue works like this. That's why the unit test is commented so much.
Thanks already :)

Contributor Info
Paul Oberosler, Individual

astrogeco

Hi, thanks for the contribution!

Can you change the c++ style comments // to use /* blocks?

paulober · 2021-07-28T10:30:26Z

Can you change the c++ style comments // to use /* blocks?

Done...

astrogeco · 2021-07-28T13:51:39Z

@PavLL looks like you still have some checks failing, I recommend "enabling actions" in your fork.

modules/cfe_testcase/src/time_conversion_test.c

modules/cfe_testcase/src/cfe_test.c

modules/cfe_testcase/src/time_conversion_test.c

astrogeco · 2021-07-28T16:14:25Z

CCB:2021-06-27, looking good, revisit after edits are done

modules/cfe_testcase/src/time_conversion_test.c

zanzaben

Code looks all good. Last thing you need to do is squash your commits down to 1 with the message "Fix #1690, Add Time Conversion Functional Test"

paulober · 2021-07-29T14:46:21Z

Ok, I just removed the TimeInRange again from the cfe_test.h header because I replaced it with CFE_Time_Compare in my ut.

paulober · 2021-07-29T14:56:55Z

Ok, done. I squashed the commits.

zanzaben · 2021-07-29T15:00:26Z

You can't use Time Compare there, that only returns -1, 0, or 1 so it passes as true only when the two times are different. You need to use time in range.

paulober · 2021-07-29T15:05:21Z

Ok, I revert this.

paulober · 2021-07-29T15:12:35Z

You can't use Time Compare there, that only returns -1, 0, or 1 so it passes as true only when the two times are different. You need to use time in range.

Ok, done. I now use TimeInRange again.

zanzaben · 2021-07-29T15:36:33Z

modules/cfe_testcase/src/time_conversion_test.c

+    /* write UTC into first buffer */
+    CFE_TIME_Print(timeBuf1, UTC);
+
+    UtAssert_True(TimeInRange(SCTime, UTC, difference), "TAI (%s) = MET2SCTime (%s)", timeBuf1, timeBuf2);


This should be UTC in the printout.

Ok, fixed. What a stupid mistake.

astrogeco · 2021-08-04T16:24:27Z

CCB:2021-08-04 APPROVED

nasa/cFE#1742, Remove SB get last message sender info requirement nasa/cFE#1732, Fix #1725 Update UTs to use UtAssert_MIR nasa/cFE#1736, Add Functional Tests cFE Message ID nasa/cFE#1707, Add Time Conversion Functional Test

**Combines** nasa/cFE#1759, v6.8.0-rc1+dev810 nasa/osal#1126, v5.1.0-rc1+dev586 **Includes** *cFE* - nasa/cFE#1752, Add null pointer check to table GetAddresses and ReleaseAddresses - nasa/cFE#1742, Remove SB get last message sender info requirement - nasa/cFE#1732, Fix #1725 Update UTs to use UtAssert_MIR - nasa/cFE#1736, Add Functional Tests cFE Message ID - nasa/cFE#1707, Add Time Conversion Functional Test - nasa/cFE#1739, Add cast to MIR prints *osal* - nasa/osal#1122, Add UtAssert_MIR macro - nasa/osal#1125, add generic asserts from CFE coverage testing - nasa/osal#1121, add osapi-shell-stubs.c to OSAL stub library Co-authored-by: Jacob Hageman <skliper@users.noreply.github.com> Co-authored-by: Joseph Hickey <jphickey@users.noreply.github.com> Co-authored-by: Alex Campbell <zanzaben@users.noreply.github.com> Co-authored-by: Jose F Martinez Pedraza <pepepr08@users.noreply.github.com> Co-authored-by: Niall Mullane <nmullane@users.noreply.github.com> Co-authored-by: Ariel Adams <ArielSAdamsNASA@users.noreply.github.com> Co-authored-by: Paul <pavll@users.noreply.github.com>

paulober force-pushed the fix-1690-uts-for-time-conversion-api branch 2 times, most recently from ca08831 to 5263aed Compare July 25, 2021 21:02

paulober marked this pull request as ready for review July 25, 2021 21:07

paulober changed the title ~~Fix #1690 uts for time conversion api~~ Fix #1690, uts for time conversion api Jul 25, 2021

astrogeco added CCB:Ready Ready for discussion at the Configuration Control Board (CCB) community Community contribution, YAY! labels Jul 27, 2021

astrogeco requested a review from zanzaben July 27, 2021 23:38

astrogeco suggested changes Jul 27, 2021

View reviewed changes

paulober requested a review from astrogeco July 28, 2021 12:39

astrogeco approved these changes Jul 28, 2021

View reviewed changes

zanzaben suggested changes Jul 28, 2021

View reviewed changes

paulober requested a review from zanzaben July 28, 2021 17:37

zanzaben suggested changes Jul 28, 2021

View reviewed changes

modules/cfe_testcase/src/time_conversion_test.c Outdated Show resolved Hide resolved

modules/cfe_testcase/src/time_conversion_test.c Outdated Show resolved Hide resolved

paulober requested a review from zanzaben July 28, 2021 19:29

astrogeco changed the title ~~Fix #1690, uts for time conversion api~~ Fix #1690, Unit Tests for time conversion api Jul 29, 2021

astrogeco changed the title ~~Fix #1690, Unit Tests for time conversion api~~ Fix #1690, Tests for time conversion api Jul 29, 2021

zanzaben suggested changes Jul 29, 2021

View reviewed changes

paulober force-pushed the fix-1690-uts-for-time-conversion-api branch 2 times, most recently from 6278928 to 8274e59 Compare July 29, 2021 14:56

paulober requested a review from zanzaben July 29, 2021 14:57

paulober force-pushed the fix-1690-uts-for-time-conversion-api branch from 83d3476 to 0ea4ab8 Compare July 29, 2021 15:11

zanzaben suggested changes Jul 29, 2021

View reviewed changes

paulober force-pushed the fix-1690-uts-for-time-conversion-api branch from 0ea4ab8 to a0a23aa Compare July 29, 2021 15:51

paulober requested a review from zanzaben July 29, 2021 15:52

paulober force-pushed the fix-1690-uts-for-time-conversion-api branch from a0a23aa to f4510c4 Compare July 29, 2021 15:55

Fix nasa#1690, Add Time Conversion Functional Test

9e16d16

paulober force-pushed the fix-1690-uts-for-time-conversion-api branch from f4510c4 to 9e16d16 Compare July 29, 2021 15:58

zanzaben approved these changes Jul 29, 2021

View reviewed changes

astrogeco added CCB:Approved Indicates code review and approval by community CCB and removed CCB:Ready Ready for discussion at the Configuration Control Board (CCB) labels Aug 4, 2021

astrogeco changed the base branch from main to integration-candidate August 4, 2021 22:42

astrogeco merged commit 47e591a into nasa:integration-candidate Aug 4, 2021

astrogeco mentioned this pull request Aug 4, 2021

cFE Integration candidate: 2021-08-03 #1759

Merged

astrogeco mentioned this pull request Aug 6, 2021

cFS Integration Candidate: 2021-08-03 nasa/cFS#328

Merged

skliper added this to the 7.0.0 milestone Sep 24, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #1690, Tests for time conversion api #1707

Fix #1690, Tests for time conversion api #1707

paulober commented Jul 25, 2021 •

edited

Loading

astrogeco left a comment

paulober commented Jul 28, 2021

astrogeco commented Jul 28, 2021

astrogeco commented Jul 28, 2021

zanzaben left a comment

paulober commented Jul 29, 2021

paulober commented Jul 29, 2021

zanzaben commented Jul 29, 2021

paulober commented Jul 29, 2021

paulober commented Jul 29, 2021

zanzaben Jul 29, 2021 •

edited

Loading

paulober Jul 29, 2021

astrogeco commented Aug 4, 2021

Fix #1690, Tests for time conversion api #1707

Fix #1690, Tests for time conversion api #1707

Conversation

paulober commented Jul 25, 2021 • edited Loading

astrogeco left a comment

Choose a reason for hiding this comment

paulober commented Jul 28, 2021

astrogeco commented Jul 28, 2021

astrogeco commented Jul 28, 2021

zanzaben left a comment

Choose a reason for hiding this comment

paulober commented Jul 29, 2021

paulober commented Jul 29, 2021

zanzaben commented Jul 29, 2021

paulober commented Jul 29, 2021

paulober commented Jul 29, 2021

zanzaben Jul 29, 2021 • edited Loading

Choose a reason for hiding this comment

paulober Jul 29, 2021

Choose a reason for hiding this comment

astrogeco commented Aug 4, 2021

paulober commented Jul 25, 2021 •

edited

Loading

zanzaben Jul 29, 2021 •

edited

Loading