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

Also show "investigation" tabs for incomplete jobs #3981

Merged
merged 2 commits into from
Jan 25, 2022
Merged

Also show "investigation" tabs for incomplete jobs #3981

merged 2 commits into from
Jan 25, 2022

Conversation

okurz
Copy link
Member

@okurz okurz commented Jun 28, 2021

Also in case of incomplete jobs the problem can be caused by something
that the investigation tab could reveal, e.g. invalid test code that
causes the job to abort prematurely with incomplete.

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

@okurz okurz marked this pull request as draft June 28, 2021 11:26
Martchus
Martchus requested changes Jun 29, 2021
View reviewed changes
lib/OpenQA/Schema/Result/Jobs.pm Outdated Show resolved Hide resolved
@okurz okurz force-pushed the feature/investigate_incompletes branch 2 times, most recently from d576a05 to 171999f Compare July 6, 2021 20:54
Martchus
Martchus requested changes Jul 7, 2021
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. 

        #   Failed test 'previous job identified as last good and it is a hash'
        #   at t/10-jobs.t line 473.
        #          got: ''
        #     expected: 'HASH'
        # Looks like you failed 1 test of 4.

    #   Failed test 'additional investigation notes provided on new failed'
    #   at t/10-jobs.t line 489.
    # Looks like you failed 1 test of 10.

#   Failed test 'carry over, including soft-fails'

@okurz okurz force-pushed the feature/investigate_incompletes branch from 171999f to 85498ce Compare July 28, 2021 06:11
Martchus
Martchus requested changes Jul 28, 2021
View reviewed changes
lib/OpenQA/Schema/Result/Jobs.pm Outdated Show resolved Hide resolved
@okurz okurz force-pushed the feature/investigate_incompletes branch from 85498ce to 926f616 Compare July 28, 2021 13:31
@okurz okurz force-pushed the feature/investigate_incompletes branch from 926f616 to 4d3c745 Compare July 28, 2021 14:38
@okurz okurz force-pushed the feature/investigate_incompletes branch from 4d3c745 to d9bf1c4 Compare October 13, 2021 10:19
Martchus
Martchus requested changes Oct 13, 2021
View reviewed changes
lib/OpenQA/Jobs/Constants.pm Outdated Show resolved Hide resolved
@okurz okurz force-pushed the feature/investigate_incompletes branch from d9bf1c4 to 8bbabcb Compare October 13, 2021 11:54
@okurz
Copy link
Member Author

okurz commented Oct 13, 2021

I split out #4300 to narrow down on any failing parts (if any)

@codecov
Copy link

codecov bot commented Oct 13, 2021 •
edited
Loading

Codecov Report

Merging #3981 (f07d4f4) into master (9e0bfee) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #3981   +/-   ##
=======================================
  Coverage   97.95%   97.95%           
=======================================
  Files         374      374           
  Lines       34006    34010    +4     
=======================================
+ Hits        33312    33316    +4     
  Misses        694      694
Impacted Files Coverage Δ
lib/OpenQA/Jobs/Constants.pm 100.00% <100.00%> (ø)
lib/OpenQA/Schema/Result/Jobs.pm 98.52% <100.00%> (ø)
t/ui/18-tests-details.t 100.00% <100.00%> (ø)

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 9e0bfee...f07d4f4. Read the comment docs.

@okurz okurz force-pushed the feature/investigate_incompletes branch from 8bbabcb to e72b3ac Compare November 11, 2021 14:21
@okurz okurz force-pushed the feature/investigate_incompletes branch from e72b3ac to 2558727 Compare January 12, 2022 11:09
@okurz okurz marked this pull request as ready for review January 12, 2022 12:27
@okurz
Copy link
Member Author

okurz commented Jan 12, 2022

Ready for review, waiting for second approval

kalikiana
kalikiana previously requested changes Jan 12, 2022
View reviewed changes
Copy link
Member

@kalikiana kalikiana left a comment

Choose a reason for hiding this comment

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

Code looks nice.

Would you please also add a test for it?

assets/javascripts/test_result.js Show resolved Hide resolved
@okurz
Copy link
Member Author

okurz commented Jan 12, 2022 via email

Martchus
Martchus requested changes Jan 12, 2022
View reviewed changes
lib/OpenQA/Schema/Result/Jobs.pm Outdated Show resolved Hide resolved
@kalikiana
Copy link
Member

Would you please also add a test for it?
I don't want to

If this is a joke I don't get it.

@okurz
Copy link
Member Author

okurz commented Jan 12, 2022

If this is a joke I don't get it.

It's no joke. We already have sufficient statement coverage and the only risk I see is that the investigation tab does not show up which I covered manually

@okurz
Also show "investigation" tabs for incomplete jobs
daedcfb 
Also in case of incomplete jobs the problem can be caused by something
that the investigation tab could reveal, e.g. invalid test code that
causes the job to abort prematurely with incomplete.

Also this commit changes the hard-coded job state decision in
OpenQA::Schema::Result::Jobs to use constant function based decisions.

Related progress issue: https://progress.opensuse.org/issues/94792
@okurz okurz force-pushed the feature/investigate_incompletes branch from 2558727 to daedcfb Compare January 12, 2022 17:44
Martchus
Martchus approved these changes Jan 12, 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.

In sub investigate your version of should_show_investigation would likely make more sense but I suppose it doesn't matter because the client won't show the tab anyways until a final job state is reached.

@okurz
Copy link
Member Author

okurz commented Jan 12, 2022

In sub investigate your version of should_show_investigation would likely make more sense but I suppose it doesn't matter because the client won't show the tab anyways until a final job state is reached.

Yes. That's why it's called "should show" not "will most definitely do show" ;)

@kalikiana
Copy link
Member

It's no joke. We already have sufficient statement coverage and the only risk I see is that the investigation tab does not show up which I covered manually

You didn't mention anything about why tests can't be added, or manual testing, in the description, commit message or ticket.

Is it difficult to add? I would guess 18-tests-details.t is easy to extend? You can always ask for help.

@kalikiana
Copy link
Member

Is it difficult to add? I would guess 18-tests-details.t is easy to extend? You can always ask for help.

Oli took me up on my offer in chat, so I'll provide one shortly.

@okurz
Copy link
Member Author

okurz commented Jan 24, 2022

Is it difficult to add? I would guess 18-tests-details.t is easy to extend? You can always ask for help.

Oli took me up on my offer in chat, so I'll provide one shortly.

You can just accept this PR as is :)

@kalikiana kalikiana dismissed their stale review January 24, 2022 20:02

I added a test. Probably shouldn't review it myself.

okurz
okurz commented Jan 24, 2022
View reviewed changes
Copy link
Member Author

@okurz okurz left a comment •
edited by kalikiana
Loading

Choose a reason for hiding this comment

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

@kalikiana your changes LGTM, you can approve again

@okurz okurz merged commit 0966958 into os-autoinst:master Jan 25, 2022
@okurz okurz deleted the feature/investigate_incompletes branch January 25, 2022 05:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kalikiana kalikiana kalikiana 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.

None yet
3 participants
@okurz @kalikiana @Martchus