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

Fix tests #638

Merged
merged 4 commits into from
Apr 6, 2023
Merged

Fix tests #638

merged 4 commits into from
Apr 6, 2023

Conversation

jreidinger
Copy link
Member

Problem

build failing at https://build.opensuse.org/package/live_build_log/openSUSE:Factory:Staging:G/yast2-packager/standard/x86_64

Solution

Fix test to use proper string instead of Pathname.

Testing

  • Tested manually

@mvidner
Copy link
Member

mvidner commented Apr 6, 2023

https://github.com/yast/yast-ruby-bindings/blob/9a50d95be89f3dfe81cd109e6bf89a5fbedc48eb/src/ruby/yast/y2logger.rb#L48 shows that we're using a standard logger, and the Ruby documentation shows that passing a block will avoid evaluating it when debugging is disabled.

So please use log.debug { "nasty string #{mem_bomb}" } instead

BTW that Ruby docs is so skillful in hiding the useful information, it sucks

@jreidinger
Copy link
Member Author

@mvidner sadly block form does not work, as we do not limit access to debug in logger level but on ruby-bindings level

try yourself in irb:

1 require "yast"
  2 
  3 include Yast::Logger
  4 
  5 log.debug { puts "eval"; "log content" }

@mvidner
Copy link
Member

mvidner commented Apr 6, 2023

Ah, good point. Then what about

  1. make an Issue in ruby-bindings "log.debug { expensive } always evaluates the block"
  2. leave the calls here, commented out, referencing that issue

@jreidinger
Copy link
Member Author

issue created yast/yast-ruby-bindings#290

Copy link
Member

@mvidner mvidner left a comment

Choose a reason for hiding this comment

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

Fixes so far look good, but what about

  • the other failure "unexpected message :define_method with ("switch_to_richtext=")", do you need help?
  • version + changelog?

@coveralls
Copy link

coveralls commented Apr 6, 2023

Coverage Status

Coverage: 37.369% (-0.01%) from 37.38% when pulling c3929ca on fix_tests into 58c7753 on master.

@@ -66,7 +66,8 @@

it "does not display any popup" do
# stub empty Yast::Popup so any method call would raise an exception
stub_const("Yast::Popup", double)
Copy link
Member

Choose a reason for hiding this comment

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

Does that mean that any such stub_const("Yast::Foo", double) will fail or has bug? We have more

https://github.com/search?q=org%3Ayast+stub_const&type=code

Copy link
Member

@mvidner mvidner Apr 6, 2023

Choose a reason for hiding this comment

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

yast/yast-update/test/inst_update_partition_auto_test.rb
15: stub_const("Yast::FileSystems", double)
yast/yast-installation/test/snapshots_finish_test.rb
10: stub_const("Yast::StorageSnapper", double)
and more

Copy link
Member

@mvidner mvidner left a comment

Choose a reason for hiding this comment

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

Thanks!

I think there is still a chance that another fix will be needed in ruby-bindings because of the following, but I will not let that block this PR.

Does that mean that any such stub_const("Yast::Foo", double) will fail or has bug? We have more: https://github.com/search?q=org%3Ayast+stub_const&type=code

@jreidinger jreidinger merged commit 2d7f52b into master Apr 6, 2023
@mvidner
Copy link
Member

mvidner commented Apr 6, 2023

For the record, the stub_const fix adjusts for the change done in yast/yast-ruby-bindings#289

@jreidinger jreidinger deleted the fix_tests branch April 6, 2023 10:35
@yast-bot
Copy link
Contributor

yast-bot commented Apr 6, 2023

✔️ Public Jenkins job #207 successfully finished
✔️ Created OBS submit request #1077710

@yast-bot
Copy link
Contributor

yast-bot commented Aug 7, 2023

❌ Internal Jenkins job #123 failed

@yast-bot
Copy link
Contributor

yast-bot commented Aug 8, 2023

❌ Internal Jenkins job #124 failed

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.

4 participants