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.

Sign up for GitHub

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

Jump to bottom

Add basic backend-svirt.t test #2048

Merged
merged 3 commits into from
May 20, 2022
Merged

Add basic backend-svirt.t test #2048

merged 3 commits into from
May 20, 2022

Conversation

mimi1vx
Copy link
Member

@mimi1vx mimi1vx commented May 5, 2022 •
edited
Loading

@kalikiana kalikiana marked this pull request as draft May 5, 2022 10:04
@codecov
Copy link

codecov bot commented May 5, 2022 •
edited
Loading

Codecov Report

Merging #2048 (3fadc0a) into master (981fd51) will increase coverage by 1.42%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #2048      +/-   ##
==========================================
+ Coverage   80.09%   81.51%   +1.42%     
==========================================
  Files          70       70              
  Lines        7294     7292       -2     
==========================================
+ Hits         5842     5944     +102     
+ Misses       1452     1348     -104
Impacted Files Coverage Δ
backend/qemu.pm 58.88% <ø> (-0.07%) ⬇️
backend/svirt.pm 100.00% <100.00%> (+57.40%) ⬆️
bmwqemu.pm 100.00% <0.00%> (ø)
testapi.pm 67.05% <0.00%> (ø)
backend/amt.pm 100.00% <0.00%> (ø)
consoles/video_base.pm 92.45% <0.00%> (+0.14%) ⬆️
backend/baseclass.pm 82.88% <0.00%> (+1.30%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 981fd51...3fadc0a. Read the comment docs.

Martchus
Martchus reviewed May 5, 2022
View reviewed changes
Copy link
Contributor

@Martchus Martchus left a comment

Choose a reason for hiding this comment

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

It would be good to know how it relates to t/22-svirt.t and why you haven't extended the existing test file. Additionally, I hope you have checked that these parts aren't already covered (because testing the same code twice is counter productive as it only extends the CI runtime with no additional value).

@github-actions
Copy link

github-actions bot commented May 5, 2022 •
edited by okurz
Loading

Great PR! Please pay attention to the following items before merging:

Files matching backend/**.pm:

  • Consider running manual verification tests, especially for non-qemu backends

This is an automatically generated QA checklist based on modified files

@mimi1vx mimi1vx force-pushed the test%svirt branch 7 times, most recently from 28332c5 to 8518497 Compare May 11, 2022 14:08
okurz
okurz requested changes May 11, 2022
View reviewed changes
Copy link
Member

@okurz okurz left a comment

Choose a reason for hiding this comment

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

@mimi1vx have you seen #2048 (review) ? Maybe you are doing too much? Also please s/backent/backend/ :)

okurz
okurz requested changes May 11, 2022
View reviewed changes
t/29-backend-svirt.t Outdated Show resolved Hide resolved
@mimi1vx mimi1vx force-pushed the test%svirt branch 3 times, most recently from 4c54f18 to aaa3e46 Compare May 11, 2022 15:30
@mimi1vx mimi1vx changed the title WIP: Add basic backent-svirt.t test WIP: Add basic backend-svirt.t test May 11, 2022
@mimi1vx mimi1vx force-pushed the test%svirt branch 4 times, most recently from 7c4442a to dc9915c Compare May 18, 2022 07:48
@Martchus
Copy link
Contributor

Have you seen my previous comment?

@mimi1vx mimi1vx force-pushed the test%svirt branch 3 times, most recently from c49ab4a to 43c7575 Compare May 18, 2022 11:51
@mimi1vx mimi1vx changed the title WIP: Add basic backend-svirt.t test Add basic backend-svirt.t test May 18, 2022
@mimi1vx mimi1vx marked this pull request as ready for review May 18, 2022 11:51
@mimi1vx
Copy link
Member Author

mimi1vx commented May 18, 2022 •
edited
Loading

It would be good to know how it relates to t/22-svirt.t and why you haven't extended the existing test file.

this is unit tests like 29-backend-amt.t as proposed in poo. I think target of t/22-svirt. is different

Additionally, I hope you have checked that these parts aren't already covered (because testing the same code twice is co
cunter productive as it only extends the CI runtime with no additional value).

most of tests is new coverage

@mimi1vx
Copy link
Member Author

mimi1vx commented May 18, 2022

@Martchus any idea why https://app.codecov.io/gh/os-autoinst/os-autoinst/compare/2048/changes#D1L105 isn't reached ... ??

@mimi1vx mimi1vx requested a review from okurz May 18, 2022 12:15
@okurz
Copy link
Member

okurz commented May 18, 2022

@Martchus any idea why https://app.codecov.io/gh/os-autoinst/os-autoinst/compare/2048/changes#D1L105 isn't reached ... ??

I don't know why it's not reached but I guess you could simply turn around the return ... if to if ... return and combine the two if-statements

Martchus
Martchus requested changes May 18, 2022
View reviewed changes
t/29-backend-svirt.t Outdated Show resolved Hide resolved
t/29-backend-svirt.t Outdated Show resolved Hide resolved
Martchus
Martchus reviewed May 18, 2022
View reviewed changes
t/29-backend-svirt.t Outdated Show resolved Hide resolved
@mimi1vx mimi1vx requested a review from Martchus May 18, 2022 14:53
Martchus
Martchus requested changes May 18, 2022
View reviewed changes
Copy link
Contributor

@Martchus Martchus left a comment

Choose a reason for hiding this comment

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

Please rename Small fixes for svirt backend and removal of HDDFORMAT check to match our formatting style (start with a verb in imperative).

backend/svirt.pm Outdated Show resolved Hide resolved
t/29-backend-svirt.t Outdated Show resolved Hide resolved
t/29-backend-svirt.t Outdated Show resolved Hide resolved
t/29-backend-svirt.t Outdated Show resolved Hide resolved
okurz
okurz requested changes May 19, 2022
View reviewed changes
Copy link
Member

@okurz okurz left a comment

Choose a reason for hiding this comment

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

See comments by Martchus

@mergify mergify bot merged commit 75eb1b1 into os-autoinst:master May 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@okurz okurz okurz approved these changes

@Martchus Martchus Martchus approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mimi1vx @Martchus @okurz