Skip to content
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

Add tests for the Time class in Fw #1736

Merged
merged 2 commits into from
Oct 27, 2022
Merged

Conversation

gazayas
Copy link
Contributor

@gazayas gazayas commented Oct 25, 2022

Originating Project/Creator Gabriel Zayas
Affected Component Fw
Affected Architectures(s) None
Related Issue(s) None
Has Unit Tests (y/n) Yes
Builds Without Errors (y/n) Yes
Unit Tests Pass (y/n) Yes
Documentation Included (y/n) No

Change Description

I added a few tests for the Time class in the Fw component.

Rationale

There was a comment indicating the tests should check the add function, but no tests had been implemented.

// test add
// test subtract
// normal subtract
Fw::Time time3 = Fw::Time::sub(time2,time1);
ASSERT_EQ(time3.m_timeBase,TB_NONE);
ASSERT_EQ(time3.m_timeContext,0);
ASSERT_EQ(time3.m_seconds,3000);
ASSERT_EQ(time3.m_useconds,1000);

Testing/Review Recommendations

The tests are pretty straight-forward, but I particularly gave attention to how I commented each test since I felt it could've used a little cleaning up.

Future Work

Apart from add I added tests for checking comparison logic, but if there are any other parts of the class that aren't covered here, it might be a good idea to test those parts as well.

@LeStarch LeStarch changed the base branch from master to devel October 27, 2022 16:19
@LeStarch
Copy link
Collaborator

This looks like a step towards better testing. Thank you!

@LeStarch
Copy link
Collaborator

This is failing on an unrelated problem that has already been fixed in devel. I am not sure why CI is unhappy. I am going to merge it and keep an eye on the post-merge build.

@LeStarch LeStarch merged commit 8855f31 into nasa:devel Oct 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants