search

flux-framework / flux-accounting

bank/accounting interface for the Flux resource manager
https://flux-framework.readthedocs.io/projects/flux-accounting/en/latest/index.html
GNU Lesser General Public License v3.0
3 stars 10 forks source link

t: skip t1011 if job-archive module not detected, add new tests for `fetch-job-records` #518

Closed cmoussa1 closed 1 month ago

cmoussa1 commented 1 month ago

Problem

Mentioned in #517, t1011-job-archive-interface.t relies on the job-archive module in flux-core which is being considered for removal in https://github.com/flux-framework/flux-core/pull/6378 since flux-accounting now has that capability on its own.

This PR adds a check in t1011-job-archive-interface.t that will skip the test file in the event that the job-archive module does not exist.

I've also added a new file to the test suite that very similarly runs through the same tests in t1011 but does not rely at all on the detection of the job-archive module.

chu11 commented 1 month ago

Why not just remove the tests that rely on job-archive, since flux-accounting is no longer using job-archive? Then perhaps this could just be one commit, saying something like "we aren't using job-archive anymore, rework to rely on ..."?

cmoussa1 commented 1 month ago

I think we thought about it briefly in the thread #517, but we are not 100% sure if there are old-enough versions of flux-accounting that are still running on our systems that still rely on the job-archive module? i.e v0.33.0 and older. Sorry if I am misunderstanding you.

garlick commented 1 month ago

My concern was that we don't necessarily want to lose coverage for reading and parsing the job-archive database if we still need to migrate some production systems to the new job table. If we're past that then agreed we can probably just drop the tests.

chu11 commented 1 month ago

If we're past that then agreed we can probably just drop the tests.

I was going with this assumption that we were long past systems using job-archive. But if we aren't, then we should keep the tests around a little longer and just create a "TODO" issue for later on.

cmoussa1 commented 1 month ago

I think I'll just go ahead and drop those tests - I don't think versions of flux-accounting v0.33.0 or older are still in use anywhere. I'll edit this PR to just have one commit!

cmoussa1 commented 1 month ago

Thanks! Setting MWP here