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

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

Merged
merged 1 commit into from
Oct 17, 2024

Conversation

cmoussa1
Copy link
Member

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
flux-framework/flux-core#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.

@cmoussa1 cmoussa1 added the testing issues that deal with testing label Oct 17, 2024
@cmoussa1 cmoussa1 requested a review from garlick October 17, 2024 17:28
@chu11
Copy link
Member

chu11 commented Oct 17, 2024

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
Copy link
Member Author

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
Copy link
Member

garlick commented Oct 17, 2024

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
Copy link
Member

chu11 commented Oct 17, 2024

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
Copy link
Member Author

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!

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

Edit the tests that use the job-archive module and just use
flux-accounting's fetch-job-records script to fetch inactive job
records.
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!

@cmoussa1
Copy link
Member Author

Thanks! Setting MWP here

@mergify mergify bot merged commit 38c9306 into flux-framework:master Oct 17, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge-when-passing testing issues that deal with testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants