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

force_cron_run: Update soft_fail reference #3947

Merged
merged 1 commit into from
Dec 2, 2017
Merged

force_cron_run: Update soft_fail reference #3947

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions tests/console/force_cron_run.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@

# Summary: Avoid suprises later and run the cron jobs explicitly
# Maintainer: Stephan Kulow <coolo@suse.de>
# Tags: bsc#1017461
# Tags: bsc#1017461, bsc#1063638
Copy link
Member

Choose a reason for hiding this comment

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

if you replace bugref why you not replace it here but just add ? old version still relevant somehow ?

and out of review question - what this "Tags" mean ? Is it just some convention or there is some mechanism in perl to handle this ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure. The old reference is a very important bug so I would like to keep a reverence to it. It's only a convention so far. I don't know of any script that is extracting it.

Copy link
Contributor

Choose a reason for hiding this comment

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

keep a reverence? That's very important - we should respekt our bugs way more :)

Copy link
Member

Choose a reason for hiding this comment

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

I just checked - your "very important" bug has state "VERIFIED FIXED" . If you insists to have it here anyway , please move it into separate line with clear explanation why it so important and how it's related to this module. Comments suppose to help people to understand code better and not just leave some side notes which can be understand by few people

Copy link
Contributor

Choose a reason for hiding this comment

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

FIXED bugs are prime reason for regression tests, what is your point?

Copy link
Member

Choose a reason for hiding this comment

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

My point is that I don't understand what this "Tags" list suppose to tell me. This module had two softfailures : bsc#1064834 , bsc#1017461 and only second one was mentioned in "Tags" . Now bsc#1017461 was removed from code and replaced with bsc#1063638 . But "Tags" suppose to keep bsc#1017461 add bsc#1063638 and keep ignoring bsc#1064834. Bug bsc#1017461 is about BTRFS but nothing in test name or comments in tests explain me how this bug related to this test. I am sure that @coolo , @okurz have a perfect picture in there head about this but I don't have it and I am in big doubt that everyone have it.

Copy link
Contributor

Choose a reason for hiding this comment

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

1017461 is the whole reason this test exists at all - 1063638 is the reason the test still exists. Clear now? The tags: comment is basically that: a comment.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah thanks , now it's clear for me but still unclear for person who will read this comment without reading all this discussion :) . So it will be nice if information from your message will migrate to comment

Copy link
Member Author

Choose a reason for hiding this comment

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

the bug is VERIFIED FIXED because I did that. A description for "Tags" could end up in the README file of the repo.

Copy link
Member Author

Choose a reason for hiding this comment

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

@coolo Yes, I guess for bsc#1017461 "reverence" is really the right word :D


use base "consoletest";
use strict;
Expand All @@ -21,7 +21,7 @@ sub settle_load {
# JeOS is different to SLE general as it extends the appliance's disk on first boot,
# so the balance is a different challenge to SLE. Elapsed time is not necessary a key
# measure here, responsiveness of the system is.
assert_screen_with_soft_timeout('top-load-decreased', soft_timeout => is_jeos() ? 180 : 30, bugref => 'bsc#1017461', timeout => 1000);
assert_screen_with_soft_timeout('top-load-decreased', soft_timeout => is_jeos() ? 180 : 30, bugref => 'bsc#1063638', timeout => 1000);
send_key 'q';
wait_serial 'TOP-DONE' || die 'top did not quit?';
}
Expand All @@ -41,7 +41,7 @@ sub run {
settle_load;
my $before = time;
assert_script_run "bash -x /usr/lib/cron/run-crons", 1000;
record_soft_failure 'bsc#1017461 - long running btrfs cron jobs can make system unresponsive' if (time - $before) > 60;
record_soft_failure 'bsc#1063638 - review I/O scheduling parameters of btrfsmaintenance' if (time - $before) > 60;
sleep 3; # some head room for the load average to rise
settle_load;

Expand Down