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

[unittests] Add unittests for SwitchStateBaseHostif #1133

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

kcudnik
Copy link
Collaborator

@kcudnik kcudnik commented Sep 22, 2022

No description provided.

@@ -46,7 +47,7 @@ int SwitchStateBase::vs_create_tap_device(

const char *tundev = "/dev/net/tun";

int fd = open(tundev, O_RDWR);
int fd = saimeta::System::open(tundev, O_RDWR);
Copy link
Contributor

Choose a reason for hiding this comment

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

open

What is the value of adding this function? Improve coverage number?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

correct

Copy link
Collaborator Author

@kcudnik kcudnik Sep 26, 2022

Choose a reason for hiding this comment

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

wrapper on system function is need to simulate function succes on unittests, it seems to me an ugly solution but i could not find better one, any suggestions ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Is the performance a concern? Does debug build help the coverage anyhow?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

there is no performance impact since those apis are not on critical paths, coverage is achieved in places required by #1110

@kcudnik
Copy link
Collaborator Author

kcudnik commented Oct 3, 2022

ut fail probably not related to the change

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants