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

Check for warnings in isotovideo test #2211

Merged
merged 1 commit into from Nov 22, 2022

Conversation

kalikiana
Copy link
Member

@kalikiana kalikiana commented Nov 21, 2022

And remove the misleading and unused output handling.

See: https://progress.opensuse.org/issues/120786

Note: This would have shown a clear error message when failing tests in #2206. It doesn't address the problem of CI behaving differently.

And remove the misleading and unused output handling.

See: https://progress.opensuse.org/issues/120786
@kalikiana kalikiana changed the title Something something something Check for warnings in isotovideo test Nov 21, 2022
@kalikiana kalikiana marked this pull request as ready for review November 21, 2022 13:38
@codecov
Copy link

codecov bot commented Nov 21, 2022

Codecov Report

Merging #2211 (7accf74) into master (dee0e60) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #2211   +/-   ##
=======================================
  Coverage   92.27%   92.27%           
=======================================
  Files         157      157           
  Lines       15218    15218           
=======================================
  Hits        14042    14042           
  Misses       1176     1176           
Impacted Files Coverage Δ
t/14-isotovideo.t 99.58% <100.00%> (ø)
t/data/tests/bar/baz.pm
t/data/tests/foo.pm 100.00% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@perlpunk
Copy link
Contributor

Note: This would have failed and shown a clear error message on #2206.

Really? I put it on top of #2206 and pushed it to my fork, and CI passed.

I still don't know why I get a failure locally and CI passes, but this PR doesn't address this :)

Copy link
Contributor

@perlpunk perlpunk left a comment

Choose a reason for hiding this comment

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

See my comment. I don't see how this helps

@kalikiana
Copy link
Member Author

See my comment. I don't see how this helps

It helps by revealing the information we used to understand the problem. As for CI I opened #2213.

@perlpunk
Copy link
Contributor

The description still says that #2206 would have failed with this.

@kalikiana
Copy link
Member Author

The description still says that #2206 would have failed with this.

I re-phrased it.

Copy link
Contributor

@perlpunk perlpunk left a comment

Choose a reason for hiding this comment

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

I don't know why removing $output as a return value from isotovideo() would be necessary but ok...

@mergify mergify bot merged commit 4c5642d into os-autoinst:master Nov 22, 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.

None yet

4 participants