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

testsuite: add multiple key job-info lookup tests #5575

Merged
merged 1 commit into from
Nov 20, 2023

Conversation

chu11
Copy link
Member

@chu11 chu11 commented Nov 20, 2023

Problem: In PR #5520 "multiple key" "flux job info" tests were removed due to a feature change in "flux job info". This removed some coverage of corner cases in job-info.lookup.

Add a new "job-info.lookup" RPC target helper tool and re-add a number of the previously removed "multiple key" tests.

@chu11 chu11 force-pushed the pr5520_add_back_coverage branch from 33d9a68 to 91a9383 Compare November 20, 2023 05:05
Copy link

codecov bot commented Nov 20, 2023

Codecov Report

Merging #5575 (91a9383) into master (926ad47) will decrease coverage by 0.01%.
The diff coverage is n/a.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5575      +/-   ##
==========================================
- Coverage   83.45%   83.45%   -0.01%     
==========================================
  Files         487      487              
  Lines       82318    82318              
==========================================
- Hits        68701    68695       -6     
- Misses      13617    13623       +6     

see 10 files with indirect coverage changes

Copy link
Member

@garlick garlick left a comment

Choose a reason for hiding this comment

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

LGTM, just a trivial change requested to error logging.

log_msg_exit ("error parsing jobid: %s", argv[1]);

if (!(keys = json_array ()))
log_err_exit ("json_array");
Copy link
Member

Choose a reason for hiding this comment

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

jansson functions don't claim to set errno so this should probably use log_msg_exit().

Comment on lines 45 to 48
if (!key)
log_err_exit ("json_string_value");
if (json_array_append_new (keys, key) < 0)
log_err_exit ("json_array_append_new");
Copy link
Member

Choose a reason for hiding this comment

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

same error handling comment

Problem: In PR flux-framework#5520 "multiple key" "flux job info" tests were
removed due to a feature change in "flux job info".  This removed
some coverage of corner cases in job-info.lookup.

Add a new "job-info.lookup" RPC target helper tool and re-add a number
of the previously removed "multiple key" tests.
@chu11 chu11 force-pushed the pr5520_add_back_coverage branch from 91a9383 to 2423896 Compare November 20, 2023 14:52
@chu11
Copy link
Member Author

chu11 commented Nov 20, 2023

thanks! tweaked and setting MWP

@mergify mergify bot merged commit a9edda8 into flux-framework:master Nov 20, 2023
30 of 31 checks passed
@chu11 chu11 deleted the pr5520_add_back_coverage branch November 21, 2023 16:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants