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

flux-job: fix wait-event -m, --match-context #5846

Merged
merged 3 commits into from Apr 1, 2024

Conversation

grondo
Copy link
Contributor

@grondo grondo commented Apr 1, 2024

This PR fixes context matching in flux job wait-event -m, --match-context, which currently returns a match if the value matches but key does not.

Problem: The `flux job wait-event --match-context` option returns a
match when the context value matches but they does not.

Fix the logic in wait_event_test_context().

Fixes flux-framework#5845
Problem: Many tests in t2231-job-info-eventlog-watch.t submit and
cancel a new job when using the same job would work fine. This makes
the test much slower than necessary.

Reuse the canceled job where possible. Rename the variable to JOBID
to avoid collision with other uses of lowercase `jobid`.
Copy link
Member

@chu11 chu11 left a comment

Choose a reason for hiding this comment

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

LGTM, only the one minor thing I note below

Comment on lines 185 to 188
# Note: in test below, foo=0 would match severity=0 in buggy version
test_expect_success 'flux job wait-event w/ bad match-context fails (bad key, good value)' '
test_must_fail fj_wait_event --match-context=foo=0 $JOBID exception
'
Copy link
Member

Choose a reason for hiding this comment

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

perhaps mention issue number?

Problem: No test in t2231-job-info-eventlog-watch.t ensures that the
flux-job wait-event -m, --match-context=KEY=VAL doesn't produce a
match when the VALUE matches but KEY does not.

Add a test.
@grondo
Copy link
Contributor Author

grondo commented Apr 1, 2024

Thanks! I've added the issue reference to the test as suggested and set MWP.

Copy link

codecov bot commented Apr 1, 2024

Codecov Report

Merging #5846 (90ec0b2) into master (ed07466) will increase coverage by 0.02%.
The diff coverage is 100.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5846      +/-   ##
==========================================
+ Coverage   83.26%   83.29%   +0.02%     
==========================================
  Files         511      511              
  Lines       82694    82690       -4     
==========================================
+ Hits        68858    68875      +17     
+ Misses      13836    13815      -21     
Files Coverage Δ
src/cmd/job/eventlog.c 88.63% <100.00%> (-0.26%) ⬇️

... and 13 files with indirect coverage changes

@mergify mergify bot merged commit 6e82f81 into flux-framework:master Apr 1, 2024
35 checks passed
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.

None yet

2 participants