From 6919bc0e1773e704f8f04a0bc34776942e37d04b Mon Sep 17 00:00:00 2001
From: Christopher Moussa <moussa1@llnl.gov>
Date: Mon, 16 Dec 2024 12:32:50 -0800
Subject: [PATCH 1/4] t: add space between "allow guest access..." test

Problem: There is no space between the test that allows guest access to
testexec in a couple of the files in the testsuite.

Add a space between the tests to match the format of the rest of the
testsuite.
---
 t/t1001-mf-priority-basic.t         | 1 +
 t/t1002-mf-priority-small-no-tie.t  | 1 +
 t/t1003-mf-priority-small-tie.t     | 1 +
 t/t1004-mf-priority-small-tie-all.t | 1 +
 t/t1005-max-jobs-limits.t           | 1 +
 t/t1008-mf-priority-update.t        | 1 +
 t/t1013-mf-priority-queues.t        | 1 +
 t/t1015-mf-priority-urgency.t       | 1 +
 t/t1019-mf-priority-info-fetch.t    | 1 +
 t/t1020-mf-priority-issue262.t      | 1 +
 t/t1021-mf-priority-issue332.t      | 1 +
 t/t1022-mf-priority-issue346.t      | 1 +
 t/t1027-mf-priority-issue376.t      | 1 +
 t/t1028-mf-priority-issue385.t      | 1 +
 t/t1029-mf-priority-default-bank.t  | 1 +
 t/t1030-mf-priority-update-queue.t  | 1 +
 t/t1031-mf-priority-issue406.t      | 1 +
 t/t1032-mf-priority-update-bank.t   | 1 +
 t/t1033-mf-priority-update-job.t    | 1 +
 19 files changed, 19 insertions(+)

diff --git a/t/t1001-mf-priority-basic.t b/t/t1001-mf-priority-basic.t
index 5bddbd74e..bd3513cd7 100755
--- a/t/t1001-mf-priority-basic.t
+++ b/t/t1001-mf-priority-basic.t
@@ -19,6 +19,7 @@ test_expect_success 'allow guest access to testexec' '
 	allow-guests = true
 	EOF
 '
+
 test_expect_success 'load multi-factor priority plugin' '
 	flux jobtap load -r .priority-default ${MULTI_FACTOR_PRIORITY}
 '
diff --git a/t/t1002-mf-priority-small-no-tie.t b/t/t1002-mf-priority-small-no-tie.t
index e0fbc9775..a2ad88df0 100755
--- a/t/t1002-mf-priority-small-no-tie.t
+++ b/t/t1002-mf-priority-small-no-tie.t
@@ -20,6 +20,7 @@ test_expect_success 'allow guest access to testexec' '
 	allow-guests = true
 	EOF
 '
+
 test_expect_success 'load multi-factor priority plugin' '
 	flux jobtap load -r .priority-default ${MULTI_FACTOR_PRIORITY}
 '
diff --git a/t/t1003-mf-priority-small-tie.t b/t/t1003-mf-priority-small-tie.t
index 0b4baa800..3d06a0811 100755
--- a/t/t1003-mf-priority-small-tie.t
+++ b/t/t1003-mf-priority-small-tie.t
@@ -20,6 +20,7 @@ test_expect_success 'allow guest access to testexec' '
 	allow-guests = true
 	EOF
 '
+
 test_expect_success 'load multi-factor priority plugin' '
 	flux jobtap load -r .priority-default ${MULTI_FACTOR_PRIORITY}
 '
diff --git a/t/t1004-mf-priority-small-tie-all.t b/t/t1004-mf-priority-small-tie-all.t
index 7b5a032a6..522957b56 100755
--- a/t/t1004-mf-priority-small-tie-all.t
+++ b/t/t1004-mf-priority-small-tie-all.t
@@ -20,6 +20,7 @@ test_expect_success 'allow guest access to testexec' '
 	allow-guests = true
 	EOF
 '
+
 test_expect_success 'load multi-factor priority plugin' '
 	flux jobtap load -r .priority-default ${MULTI_FACTOR_PRIORITY}
 '
diff --git a/t/t1005-max-jobs-limits.t b/t/t1005-max-jobs-limits.t
index a4746d9a4..ce2841dcc 100755
--- a/t/t1005-max-jobs-limits.t
+++ b/t/t1005-max-jobs-limits.t
@@ -19,6 +19,7 @@ test_expect_success 'allow guest access to testexec' '
 	allow-guests = true
 	EOF
 '
+
 test_expect_success 'load multi-factor priority plugin' '
 	flux jobtap load -r .priority-default ${MULTI_FACTOR_PRIORITY}
 '
diff --git a/t/t1008-mf-priority-update.t b/t/t1008-mf-priority-update.t
index f45566e88..6be948248 100755
--- a/t/t1008-mf-priority-update.t
+++ b/t/t1008-mf-priority-update.t
@@ -19,6 +19,7 @@ test_expect_success 'allow guest access to testexec' '
 	allow-guests = true
 	EOF
 '
+
 test_expect_success 'create flux-accounting DB' '
 	flux account -p $(pwd)/FluxAccountingTest.db create-db
 '
diff --git a/t/t1013-mf-priority-queues.t b/t/t1013-mf-priority-queues.t
index b00fafc22..37393103d 100755
--- a/t/t1013-mf-priority-queues.t
+++ b/t/t1013-mf-priority-queues.t
@@ -22,6 +22,7 @@ test_expect_success 'allow guest access to testexec' '
 	allow-guests = true
 	EOF
 '
+
 test_expect_success 'create flux-accounting DB' '
 	flux account -p $(pwd)/FluxAccountingTest.db create-db
 '
diff --git a/t/t1015-mf-priority-urgency.t b/t/t1015-mf-priority-urgency.t
index 30108c358..8388d73eb 100755
--- a/t/t1015-mf-priority-urgency.t
+++ b/t/t1015-mf-priority-urgency.t
@@ -19,6 +19,7 @@ test_expect_success 'allow guest access to testexec' '
 	allow-guests = true
 	EOF
 '
+
 test_expect_success 'load multi-factor priority plugin' '
 	flux jobtap load -r .priority-default ${MULTI_FACTOR_PRIORITY}
 '
diff --git a/t/t1019-mf-priority-info-fetch.t b/t/t1019-mf-priority-info-fetch.t
index 67eddc424..6f6dbf375 100755
--- a/t/t1019-mf-priority-info-fetch.t
+++ b/t/t1019-mf-priority-info-fetch.t
@@ -19,6 +19,7 @@ test_expect_success 'allow guest access to testexec' '
 	allow-guests = true
 	EOF
 '
+
 test_expect_success 'create flux-accounting DB' '
 	flux account -p $(pwd)/FluxAccountingTest.db create-db
 '
diff --git a/t/t1020-mf-priority-issue262.t b/t/t1020-mf-priority-issue262.t
index 767641d32..af3110633 100755
--- a/t/t1020-mf-priority-issue262.t
+++ b/t/t1020-mf-priority-issue262.t
@@ -20,6 +20,7 @@ test_expect_success 'allow guest access to testexec' '
 	allow-guests = true
 	EOF
 '
+
 test_expect_success 'create flux-accounting DB' '
 	flux account -p $(pwd)/FluxAccountingTest.db create-db
 '
diff --git a/t/t1021-mf-priority-issue332.t b/t/t1021-mf-priority-issue332.t
index 0cb6f61ef..451ab67e9 100755
--- a/t/t1021-mf-priority-issue332.t
+++ b/t/t1021-mf-priority-issue332.t
@@ -23,6 +23,7 @@ test_expect_success 'allow guest access to testexec' '
 	allow-guests = true
 	EOF
 '
+
 test_expect_success 'create flux-accounting DB' '
 	flux account -p $(pwd)/FluxAccountingTest.db create-db
 '
diff --git a/t/t1022-mf-priority-issue346.t b/t/t1022-mf-priority-issue346.t
index 18bc21cc9..886a00dcb 100755
--- a/t/t1022-mf-priority-issue346.t
+++ b/t/t1022-mf-priority-issue346.t
@@ -22,6 +22,7 @@ test_expect_success 'allow guest access to testexec' '
 	allow-guests = true
 	EOF
 '
+
 test_expect_success 'create flux-accounting DB' '
 	flux account -p $(pwd)/FluxAccountingTest.db create-db
 '
diff --git a/t/t1027-mf-priority-issue376.t b/t/t1027-mf-priority-issue376.t
index 28ba34c99..f35ec0190 100755
--- a/t/t1027-mf-priority-issue376.t
+++ b/t/t1027-mf-priority-issue376.t
@@ -21,6 +21,7 @@ test_expect_success 'allow guest access to testexec' '
 	allow-guests = true
 	EOF
 '
+
 test_expect_success 'create flux-accounting DB' '
 	flux account -p $(pwd)/FluxAccountingTest.db create-db
 '
diff --git a/t/t1028-mf-priority-issue385.t b/t/t1028-mf-priority-issue385.t
index e0287e84e..d79c08275 100755
--- a/t/t1028-mf-priority-issue385.t
+++ b/t/t1028-mf-priority-issue385.t
@@ -21,6 +21,7 @@ test_expect_success 'allow guest access to testexec' '
 	allow-guests = true
 	EOF
 '
+
 test_expect_success 'create flux-accounting DB, start flux-accounting service' '
 	flux account -p $(pwd)/FluxAccountingTest.db create-db &&
 	flux account-service -p ${DB_PATH} -t
diff --git a/t/t1029-mf-priority-default-bank.t b/t/t1029-mf-priority-default-bank.t
index b10e9b59d..7115be87a 100755
--- a/t/t1029-mf-priority-default-bank.t
+++ b/t/t1029-mf-priority-default-bank.t
@@ -19,6 +19,7 @@ test_expect_success 'allow guest access to testexec' '
 	allow-guests = true
 	EOF
 '
+
 test_expect_success 'load multi-factor priority plugin' '
 	flux jobtap load -r .priority-default ${MULTI_FACTOR_PRIORITY}
 '
diff --git a/t/t1030-mf-priority-update-queue.t b/t/t1030-mf-priority-update-queue.t
index d0c4d1c11..6049a4205 100755
--- a/t/t1030-mf-priority-update-queue.t
+++ b/t/t1030-mf-priority-update-queue.t
@@ -22,6 +22,7 @@ test_expect_success 'allow guest access to testexec' '
 	allow-guests = true
 	EOF
 '
+
 test_expect_success 'create flux-accounting DB' '
 	flux account -p $(pwd)/FluxAccountingTest.db create-db
 '
diff --git a/t/t1031-mf-priority-issue406.t b/t/t1031-mf-priority-issue406.t
index e96a45ce1..2443e6167 100755
--- a/t/t1031-mf-priority-issue406.t
+++ b/t/t1031-mf-priority-issue406.t
@@ -19,6 +19,7 @@ test_expect_success 'allow guest access to testexec' '
 	allow-guests = true
 	EOF
 '
+
 test_expect_success 'create flux-accounting DB' '
 	flux account -p $(pwd)/FluxAccountingTest.db create-db
 '
diff --git a/t/t1032-mf-priority-update-bank.t b/t/t1032-mf-priority-update-bank.t
index 3c00b8fe8..33946b5fa 100755
--- a/t/t1032-mf-priority-update-bank.t
+++ b/t/t1032-mf-priority-update-bank.t
@@ -21,6 +21,7 @@ test_expect_success 'allow guest access to testexec' '
 	allow-guests = true
 	EOF
 '
+
 test_expect_success 'create flux-accounting DB' '
 	flux account -p $(pwd)/FluxAccountingTest.db create-db
 '
diff --git a/t/t1033-mf-priority-update-job.t b/t/t1033-mf-priority-update-job.t
index 9687bff2f..68d9a1ca4 100755
--- a/t/t1033-mf-priority-update-job.t
+++ b/t/t1033-mf-priority-update-job.t
@@ -22,6 +22,7 @@ test_expect_success 'allow guest access to testexec' '
 	allow-guests = true
 	EOF
 '
+
 test_expect_success 'create flux-accounting DB' '
 	flux account -p $(pwd)/FluxAccountingTest.db create-db
 '

From 4533135be4d37668302af1e6ba67c8d717322c31 Mon Sep 17 00:00:00 2001
From: Christopher Moussa <moussa1@llnl.gov>
Date: Mon, 16 Dec 2024 13:30:12 -0800
Subject: [PATCH 2/4] t: add comments for what is happening in tests

Problem: Some of the tests in the testsuite can be somewhat complex and
simulate a scenario that might take multiple steps; it would be helpful
if descriptions were added that went into more detail than what is
described in the one-liner for each test.

Add some more thorough test descriptions in a couple of the test files
explaining what is being tested, particularly for the scenarios that
are broken up over multiple tests.
---
 t/t1001-mf-priority-basic.t | 11 +++++++++++
 t/t1005-max-jobs-limits.t   |  7 +++++++
 t/t1012-mf-priority-load.t  | 16 ++++++++++++++++
 3 files changed, 34 insertions(+)

diff --git a/t/t1001-mf-priority-basic.t b/t/t1001-mf-priority-basic.t
index bd3513cd7..ada5d6c94 100755
--- a/t/t1001-mf-priority-basic.t
+++ b/t/t1001-mf-priority-basic.t
@@ -97,6 +97,10 @@ test_expect_success 'update plugin with sample test data' '
 	flux python fake_payload.py
 '
 
+# The following set of tests will check the integer priority calculated by the
+# priority plugin for two associations. The fair-share of the first association
+# is 0.45321 and the fair-share of the second is 0.11345. In addition, job
+# priorities get affected if the user sets a custom urgency on their job.
 test_expect_success 'submit a job with default urgency' '
 	jobid=$(flux submit --setattr=system.bank=account3 -n1 hostname) &&
 	flux job wait-event -f json ${jobid} priority | jq '.context.priority' > job1.test &&
@@ -157,6 +161,9 @@ test_expect_success 'submit a job using default bank' '
 	flux cancel ${jobid}
 '
 
+# The following two tests ensure job submissions are rejected when a user
+# tries to submit a job under a bank they do not belong to or when the format
+# of the bank is not valid.
 test_expect_success 'submit a job using a bank the user does not belong to' '
 	test_must_fail flux submit --setattr=system.bank=account1 -n1 hostname > bad_bank.out 2>&1 &&
 	test_debug "cat bad_bank.out" &&
@@ -169,6 +176,10 @@ test_expect_success 'reject job when invalid bank format is passed in' '
 	grep "unable to unpack bank arg" invalid_fmt.out
 '
 
+# This set of tests simulates a special case where a portion of the Association
+# object that is kept with every job is missing, and therefore, an exception on
+# the job is raised with a message explaining that it cannot load the
+# information for the job.
 test_expect_success 'pass special key to user/bank struct to nullify information' '
 	cat <<-EOF >null_struct.json
 	{
diff --git a/t/t1005-max-jobs-limits.t b/t/t1005-max-jobs-limits.t
index ce2841dcc..f398ab357 100755
--- a/t/t1005-max-jobs-limits.t
+++ b/t/t1005-max-jobs-limits.t
@@ -86,6 +86,13 @@ test_expect_success 'add a default queue and send it to the plugin' '
 	flux python fake_payload.py
 '
 
+# The following set of tests will check that the priority plugin enforces
+# accounting limits defined per-association in the flux-accounting database,
+# specifically an association's max running jobs and max active jobs limit.
+# When the association hits their max running jobs limit, subsequently
+# submitted jobs will be held by having an accounting-specific dependency added
+# to it. Once they hit their max active jobs limit, subsequently submitted jobs
+# will be rejected with an accounting-specific rejection message.
 test_expect_success 'submit max number of jobs' '
 	jobid1=$(flux python ${SUBMIT_AS} 5011 sleep 60) &&
 	jobid2=$(flux python ${SUBMIT_AS} 5011 sleep 60)
diff --git a/t/t1012-mf-priority-load.t b/t/t1012-mf-priority-load.t
index 04e307c50..58f5bfbc8 100755
--- a/t/t1012-mf-priority-load.t
+++ b/t/t1012-mf-priority-load.t
@@ -75,6 +75,14 @@ test_expect_success 'create fake_payload.py' '
 	EOF
 '
 
+# The following test simulates the following scenario: a user submits a job to
+# a bank they do not have access to with the priority plugin loaded BEFORE it
+# is updated with flux-accounting information (i.e the plugin knows nothing
+# about which users belong to which bank). The plugin is then updated with the
+# flux-accounting information, and while looping through the job.state.priority
+# callback for each job, discovers that this submitted job comes from a user
+# who does not have access to the bank they submitted this job under.
+# Therefore, an exception is raised on the job.
 test_expect_success 'submitting a job specifying an incorrect bank with no user data results in a job exception' '
 	jobid0=$(flux submit --setattr=system.bank=account4 -n1 sleep 60) &&
 	flux python fake_payload.py &&
@@ -88,6 +96,11 @@ test_expect_success 'unload and reload mf_priority.so' '
 	flux jobtap list | grep mf_priority
 '
 
+# The following set of tests simulates an association submitting a job while
+# the priority plugin is loaded BEFORE it is updated with flux-accounting
+# information. The plugin holds the job while it waits to receive accounting
+# information, and once it is updated, will annotate the job with the bank name
+# and allow it to proceed to run.
 test_expect_success 'submit sleep 60 jobs with no data update' '
 	jobid1=$(flux submit -n1 sleep 60)
 '
@@ -131,6 +144,9 @@ test_expect_success 'unload mf_priority.so' '
 	flux jobtap remove mf_priority.so
 '
 
+# The following set of tests makes sure that if a job is held before the plugin
+# is successfully loaded and is updated with flux-accounting information, the
+# job can still successfully transition to RUN state.
 test_expect_success 'submit a job with no plugin loaded' '
 	jobid4=$(flux submit -n 1 sleep 60) &&
 	flux job wait-event -vt 60 ${jobid4} depend

From 39221e27a6376b42dbc3f337f73ed5799f2e7825 Mon Sep 17 00:00:00 2001
From: Christopher Moussa <moussa1@llnl.gov>
Date: Mon, 16 Dec 2024 13:33:46 -0800
Subject: [PATCH 3/4] t1005: update test description

Problem: There is a test in t1005-max-jobs-limits.t that submits two
jobs to the priority plugin under the same bank using two different
methods: by default and by explicitly setting it with --setattr.
However, the test description is confusing and could be worded better.

Update the test description to more concisely describe what is
happening in the test.
---
 t/t1005-max-jobs-limits.t | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t1005-max-jobs-limits.t b/t/t1005-max-jobs-limits.t
index f398ab357..f57f95bb8 100755
--- a/t/t1005-max-jobs-limits.t
+++ b/t/t1005-max-jobs-limits.t
@@ -125,7 +125,7 @@ test_expect_success 'a submitted job while at max-running-jobs limit will have a
 	flux cancel ${jobid2}
 '
 
-test_expect_success 'submit max number of jobs with a mix of default bank and explicitly set bank' '
+test_expect_success 'submit max number of jobs to the same bank (explicitly and by default)' '
 	jobid1=$(flux python ${SUBMIT_AS} 5011 sleep 60) &&
 	jobid2=$(flux python ${SUBMIT_AS} 5011 --setattr=system.bank=account3 -n 1 sleep 60)
 '

From 3f6e1ea9efbbf90711557e61c92a626ccd78ea5d Mon Sep 17 00:00:00 2001
From: Christopher Moussa <moussa1@llnl.gov>
Date: Mon, 16 Dec 2024 13:36:37 -0800
Subject: [PATCH 4/4] t1011: update comment above test

Problem: One of the tests in t1011-job-archive-interface.t has a
lengthy comment explaining in more detail what is being checked in the
test itself, but it is missing an important "not" when describing the
effect on the job usage value for an association.

Fix this comment by adding a "not" to the sentence.
---
 t/t1011-job-archive-interface.t | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t1011-job-archive-interface.t b/t/t1011-job-archive-interface.t
index 0cec115b3..69710e640 100755
--- a/t/t1011-job-archive-interface.t
+++ b/t/t1011-job-archive-interface.t
@@ -139,7 +139,7 @@ test_expect_success 'check that job usage and fairshare values get updated' '
 '
 
 # if update-usage is called in the same half-life period when no jobs are found
-# for a user, their job usage factor should be affected; this test is taken
+# for a user, their job usage factor should not be affected; this test is taken
 # from the set of job-archive interface Python unit tests
 test_expect_success 'call update-usage in the same half-life period where no jobs are run' '
 	flux account -p ${DB_PATH} update-usage &&