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

[AN-142] Adding cost centaur test #7685

Open
wants to merge 24 commits into
base: develop
Choose a base branch
from
Open

Conversation

lucymcnatt
Copy link
Contributor

@lucymcnatt lucymcnatt commented Jan 31, 2025

Description

  • Adds the cost endpoint to the cromwell API and the scaffolding to call it within a centaur test
  • Adds 2 test cases to the centaur suite
    • Check the cost of a workflow is within the expected range
    • Check that the cost of a workflow is 0 if call-cached
  • Removes test cases for outdated mysql versions --> official support removed for anything below 8.4

Release Notes Confirmation

CHANGELOG.md

  • I updated CHANGELOG.md in this PR
  • I assert that this change shouldn't be included in CHANGELOG.md because it doesn't impact community users

Terra Release Notes

  • I added a suggested release notes entry in this Jira ticket
  • I assert that this change doesn't need Jira release notes because it doesn't impact Terra users

@lucymcnatt lucymcnatt marked this pull request as ready for review February 5, 2025 16:00
@lucymcnatt lucymcnatt requested a review from a team as a code owner February 5, 2025 16:00
} yield ()
}

def validateNoCost(submittedWorkflow: SubmittedWorkflow): Test[Unit] =
Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe overkill, but I also added a test to verify that the cost is 0 when a workflow is call-cached

Copy link
Collaborator

Choose a reason for hiding this comment

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

I like it!

@@ -0,0 +1,17 @@
In order to run a Cromwell instance locally, there are the some prerequisites:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is just a start from what I can remember, feel free to add/edit!

Copy link
Collaborator

@jgainerdewar jgainerdewar left a comment

Choose a reason for hiding this comment

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

This is really great, very nice job 🎉 Left some minor feedback and thoughts but the missing newlines and the reference.conf change are the only real problems.

} yield ()
}

def validateNoCost(submittedWorkflow: SubmittedWorkflow): Test[Unit] =
Copy link
Collaborator

Choose a reason for hiding this comment

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

I like it!

centaur/src/main/scala/centaur/test/Test.scala Outdated Show resolved Hide resolved
core/src/main/resources/reference.conf Outdated Show resolved Hide resolved
@@ -337,6 +357,15 @@ object TestFormulas extends StrictLogging {
case _ => Test.invalidTestDefinition("Configuration not supported by PapiUpgradeTest", workflowDefinition)
}

def runSuccessfulWorkflowAndVerifyCost(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Did you consider updating the existing test format to optionally check cost (if it's provided in the test config)?

@jgainerdewar
Copy link
Collaborator

Approving assuming we can get the tests passing :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants