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

Carry over labels from previous jobs in same scenario if still failing #564

Closed
wants to merge 4 commits into from
Closed

Carry over labels from previous jobs in same scenario if still failing #564

wants to merge 4 commits into from

Conversation

okurz
Copy link
Member

@okurz okurz commented Feb 22, 2016

It is possible to label all failing tests but tedious to do by a human user
as many failures are just having the same issue until it gets fixed.
It helps if a label is preserved for a build that is still failing. This
idea is inspired by
https://wiki.jenkins-ci.org/display/JENKINS/Claim+plugin

The scheduler might not be a good location for "_carry_over_labels" so this
method could also be moved elsewhere.

Related issue: https://progress.opensuse.org/issues/10212

@okurz
Copy link
Member Author

okurz commented Feb 22, 2016

@aaannz @coolo do you suggest to move the "_carry_over_labels" somewhere else than scheduler? If yes, where?

aaannz
aaannz reviewed Feb 22, 2016
View reviewed changes
lib/OpenQA/Scheduler/Scheduler.pm
@@ -669,6 +669,44 @@ sub _job_stop_children {
}
}

sub _carry_over_labels {
my ($job) = @_;
if ($job->comments > 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to hit this condition? When job is set as !PASSED then comments are carried over, right? So without some good chunk of timing luck, you don't even have chance to add any new comments. Or did I miss something?

Copy link
Member Author

Choose a reason for hiding this comment

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

you are right, this would only hit under special circumstances reg. timing. You could consider this defensive programming. It won't harm and I could easily remove this or try to come up with the cornercase in a test if possible it all ;-) … ok will remove it for now.

@okurz okurz force-pushed the feature/comment_carry_over branch from 5c73838 to 0e64e5b Compare February 23, 2016 09:47
@okurz
Copy link
Member Author

okurz commented Feb 23, 2016

updated:

  • Move comment for "limit_previous" to its own line to prevent getting lost in right-scrolling
  • Delete statement never hit for "comments already present"
  • Do not carry over labels crossing a passed result
  • Add more tests for different state changes

aaannz
aaannz reviewed Feb 24, 2016
View reviewed changes
t/17-labels_carry_over.t
@@ -0,0 +1,110 @@
# Copyright (C) 2016 SUSE Linux GmbH
Copy link
Contributor

Choose a reason for hiding this comment

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

Its SUSE LLC now.

Copy link
Member Author

Choose a reason for hiding this comment

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

are you sure:

$ git grep -h Copyright | sort | uniq --count | sort -n | tail -n 10
      2 # Copyright (C) 2016 Red Hat
      2 # Copyright (C) 2016 SUSE Linux GmbH
      3 # Copyright (C) 2015 Red Hat
      3 # Copyright (C) 2016 SUSE LLC
      6 # Copyright (c) 2015 SUSE LINUX GmbH, Nuernberg, Germany.
      8  * Copyright jQuery Foundation and other contributors
     10 # Copyright (C) 2015 SUSE LLC
     11 # Copyright (C) 2015 SUSE Linux Products GmbH
     35 # Copyright (C) 2015 SUSE Linux GmbH
     92 # Copyright (C) 2014 SUSE Linux Products GmbH

;-P

I would not mind a copyright checker script, until then …

Copy link
Member

Choose a reason for hiding this comment

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

SUSE Linux GmbH and SUSE LLC are both valid

SUSE Linux Products GmbH is no longer valid
On 24 Feb 2016 7:18 pm, "Oliver Kurz" notifications@github.com wrote:

In t/17-labels_carry_over.t
#564 (comment):

@@ -0,0 +1,110 @@
+# Copyright (C) 2016 SUSE Linux GmbH

are you sure:

$ git grep -h Copyright | sort | uniq --count | sort -n | tail -n 10
2 # Copyright (C) 2016 Red Hat
2 # Copyright (C) 2016 SUSE Linux GmbH
3 # Copyright (C) 2015 Red Hat
3 # Copyright (C) 2016 SUSE LLC
6 # Copyright (c) 2015 SUSE LINUX GmbH, Nuernberg, Germany.
8 * Copyright jQuery Foundation and other contributors
10 # Copyright (C) 2015 SUSE LLC
11 # Copyright (C) 2015 SUSE Linux Products GmbH
35 # Copyright (C) 2015 SUSE Linux GmbH
92 # Copyright (C) 2014 SUSE Linux Products GmbH

;-P

I would not mind a copyright checker script, until then …

—
Reply to this email directly or view it on GitHub
https://github.com/os-autoinst/openQA/pull/564/files#r53981489.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, @lnussel initiated discussion with legal about that and response was:

SUSE LLC is the owner of SUSE IP. You should use it everywhere. But
don't delete but rather append.

Then there was this #441 (see outdated diff) and since then we started replacing all to SUSE LLC. Not all at once, of course, only when file is changed.

Copy link
Contributor

Choose a reason for hiding this comment

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

SUSE LLC is the one we should use for new code. SUSE Linux GmbH only exists to pay our bills

Copy link
Member Author

Choose a reason for hiding this comment

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

we agreed @aaannz will take a look into checking this automatically.

@coolo

  1. wanna keep as is and merge
  2. I change this single line with a very cost-inefficient commit fixup when other files are still wrong
  3. you have more suggestions how to improve, e.g. regarding the code in Scheduler
    ?

@okurz okurz force-pushed the feature/comment_carry_over branch from d5e8959 to c47fce9 Compare February 24, 2016 18:20
@okurz
Copy link
Member Author

okurz commented Feb 24, 2016

updated:

  • rebased

okurz added 4 commits February 24, 2016 22:12
@okurz
Extract 'bugref' and 'label' helpers into methods on comment
0986432
@okurz
Carry over labels from previous jobs in same scenario if still failing
192155a 
It is possible to label all failing tests but tedious to do by a human user
as many failures are just having the same issue until it gets fixed.
It helps if a label is preserved for a build that is still failing. This
idea is inspired by
https://wiki.jenkins-ci.org/display/JENKINS/Claim+plugin

The scheduler might not be a good location for "_carry_over_labels" so this
method could also be moved elsewhere.

Related issue: https://progress.opensuse.org/issues/10212
@okurz
Do not carry over labels over passes
40303a6 
After a job passed a new issue in a subsequent fail is assumed to be failed
for a different reason.

The test is restructured into subtests for the individual scenarios.

For restarting the jobs in tests the test detail templates showed an error
because of undefined job start time so we just set it to zero duration in
these cases.
@okurz
t: Add more tests for labels_carry_over for a complete coverage
e110f86
@okurz okurz force-pushed the feature/comment_carry_over branch from c47fce9 to e110f86 Compare February 24, 2016 21:27
@coolo
Copy link
Contributor

coolo commented Apr 13, 2016

rebased this as #641

@coolo coolo closed this Apr 13, 2016
@okurz okurz deleted the feature/comment_carry_over branch April 25, 2016 07:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@okurz @coolo @sysrich @aaannz