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

Prevent error about undefined value in next_previous route #4574

Merged

Conversation

okurz
Copy link
Member

@okurz okurz commented Mar 22, 2022

@codecov
Copy link

codecov bot commented Mar 22, 2022

Codecov Report

Merging #4574 (7697ecd) into master (8e63f78) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #4574   +/-   ##
=======================================
  Coverage   97.98%   97.98%           
=======================================
  Files         375      375           
  Lines       34332    34332           
=======================================
  Hits        33640    33640           
  Misses        692      692           
Impacted Files Coverage Δ
lib/OpenQA/WebAPI/Controller/Test.pm 94.51% <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 8e63f78...7697ecd. Read the comment docs.

@okurz okurz force-pushed the fix/cant_call_id_on_undefined_poo108662 branch from f071dd2 to 7697ecd Compare March 22, 2022 08:43
@@ -428,7 +428,7 @@ sub _show {
}

sub job_next_previous_ajax ($self) {
my $main_job = $self->get_current_job;
return $self->reply->not_found unless my $main_job = $self->_get_current_job;
Copy link
Member

Choose a reason for hiding this comment

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

Would you mind extending our unit tests to cover 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.

I consider this not necessary, it's just basic in-line safety checks which doesn't even touch coverage. Please also see #4575 where I try to make the overall code simpler with exception handling so that we don't need these checks at all.

Copy link
Member

Choose a reason for hiding this comment

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

I don't follow. I'm asking if we can have a regression test since this issue was only seen in production.

Copy link
Contributor

Choose a reason for hiding this comment

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

I also think a test would be nice

@mergify mergify bot merged commit 6f3cb49 into os-autoinst:master Mar 22, 2022
@okurz okurz deleted the fix/cant_call_id_on_undefined_poo108662 branch March 22, 2022 13:00
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